Do not unnecessarily pass post ID to various query loop aware functions #45828
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In the performance tests CI workflow we have been running every test suite three times for each branch under test. The goal of this work, introduced in #33710, was to reduce variation in the reported data from the tests. Unfortunately after measuring the data produced by our test runs, and by running experiments that run the test suites thirty times over, the overall variation is explained primarily by noise in the Github Actions container running our jobs. If running the test suites three times each reduces the variation in the results then it's not detectable above the amount of variation introduced beyond our control. Because these additional rounds extend the perf-test runtime by around twenty minutes on each PR we're reducing the number of rounds to a single pass for PR commits. This will free up compute resources and remove the performance tests as a bottleneck in the PR workflow. Additional work can and should be done to further remove variance in the testing results, but this practical step will remove an obstacle from developer iteration speed without reducing the quality of the numbers being reported.
felixarntz
added
[Type] Enhancement
|
|
felixarntz
commented
Nov 16, 2022
| @@ -35,7 +30,7 @@ function render_block_core_post_title( $attributes, $content, $block ) { | |||
|
|
|||
| if ( isset( $attributes['isLink'] ) && $attributes['isLink'] ) { | |||
| $rel = ! empty( $attributes['rel'] ) ? 'rel="' . esc_attr( $attributes['rel'] ) . '"' : ''; | |||
| $title = sprintf( '<a href="%1$s" target="%2$s" %3$s>%4$s</a>', get_the_permalink( $post_ID ), esc_attr( $attributes['linkTarget'] ), $rel, $title ); | |||
| $title = sprintf( '<a href="%1$s" target="%2$s" %3$s>%4$s</a>', get_the_permalink(), esc_attr( $attributes['linkTarget'] ), $rel, $title ); | |||
There was a problem hiding this comment.
I wonder why this change wasn't already present before. Is it an oversight or is there an intentional difference in behavior for why this function should still have the ID passed?
felixarntz
commented
Nov 16, 2022
Mamaduka
reviewed
Nov 17, 2022
…45005) * Add drawer icons to icons package Switch editor sidebar icons to new drawer icons Icon should reflect RTL as well. Update TabPanel to allow icons on TabButtons Add menu group to InspectorControl slot fills Move nav menu controls into InspectorControls menu group Introduce menu, settings & appearance tabs to sidebar Refactor InspectorControlTabs Re-orders the appearance and settings tabs. Also now omits the TabPanel altogether if only a single tab has contents. Make block inspector tabs a Gutenberg experiment A little tidy up Clean up conditional tabs display Remove nav specific menu tab for now Remove Menu inspector controls group Refactor inspector controls tabs to components Remove conditional display of tabs Render no settings or tools messages when no fills Reduce new styles for equal width tabs Add general slot for items applying to block as a whole Move query block new post link to new slot Revert "Move query block new post link to new slot" This reverts commit 1279985. Revert "Add general slot for items applying to block as a whole" This reverts commit 186276f. Tweak no style options message Add changelog for TabPanel updates Remove empty readme until experiment settles Fix copy and paste error * Fix changelog Co-authored-by: Daniel Richards <daniel.richards@automattic.com>
* Fix for navigation anchor links to close modal * Rename function to be more accurate
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com> Co-authored-by: Miguel Torres <miguel.torres@automattic.com> Co-authored-by: Jonny Harris <spacedmonkey@users.noreply.github.com>
When running the performance test suite we currently build three copies of the plugin: one for each branch under test and also one for the branch which provides the actual test suite. In this patch we're checking first if one of the branches under test is the same as that tests branch, and if so, re-using the built files from that to avoid the additional approximately five minutes of building.
We don't use the types we build during the test runs, but generating them performs additional wasted work. Let's skip over that step to cut out unecessary delay and to avoid computation we don't need.
…#45727) * Storybook: Add source link button * Remove storysource addon * Update docs * Add link to relevant plugin
* Add drawer icons to icons package Switch editor sidebar icons to new drawer icons Icon should reflect RTL as well. Update TabPanel to allow icons on TabButtons Add menu group to InspectorControl slot fills Move nav menu controls into InspectorControls menu group Introduce menu, settings & appearance tabs to sidebar Refactor InspectorControlTabs Re-orders the appearance and settings tabs. Also now omits the TabPanel altogether if only a single tab has contents. Make block inspector tabs a Gutenberg experiment A little tidy up Clean up conditional tabs display Remove nav specific menu tab for now Remove Menu inspector controls group Refactor inspector controls tabs to components Remove conditional display of tabs Render no settings or tools messages when no fills Reduce new styles for equal width tabs Add general slot for items applying to block as a whole Move query block new post link to new slot Revert "Move query block new post link to new slot" This reverts commit 1279985. Revert "Add general slot for items applying to block as a whole" This reverts commit 186276f. Tweak no style options message Add changelog for TabPanel updates Remove empty readme until experiment settles Fix copy and paste error Provide list view tab and slot for nav block * change white to allow Co-authored-by: Ben Dwyer <ben@scruffian.com>
* Lodash: Refactor away from _.pick() in block editor * Add a fallback in case of no attributes
* Include offcanvas specific styles * Use standard Inserter only * Avoid using underscore naming convention * Use standard UI components for keyboard navigation * Extract appender to file * Apply TreeGridRow props * Remove clientId prop drilling * Remove need to pass clientID to offcanvas component
* [Patterns]: Update pattern category descriptions * add hyphen
felixarntz
requested review from
spacedmonkey,
ellatrix,
fluiddot,
ntwb,
nerrad,
tellthemachines,
geriux,
ryanwelcher,
juanmaguitar and
fabiankaegy
as code owners
|
@Mamaduka Thanks for the ping, fixed! Update: Wait, now I'm confused. Now there are ton of code changes in here after rebasing - not sure what happened there 🤔 |
|
Size Change: +14 kB (+1%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
|
Hello, Can we have an update over this PR? This PR would solve the issue mentioned here - #46417. Thanks. |
7 tasks
|
Hi, @felixarntz Can you resolve merge conflicts when you have time? Maybe we can try and land this in WP 6.4. Side note: We'll need to check that changes here will work with post previews. |
annezazu
added
[Feature] Site Editor
|
Personally I think the global $post should be abandoned and only used where really necessary....
|
|
Hi, what is the state of this PR? Can it be closed or refreshed? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
This is a follow up to #37622. See #45534 (comment) for context. For the blocks updated here, they are run in the loop with the
$postglobal populated, so it should not be necessary to pass the post ID to these functions anymore. This also means they strictly don't need to have thepostIdcontext anymore.Why?
This is probably not strictly necessary, but it brings the code in line with the change from #37622.
cc @Mamaduka