#53675 closed enhancement (fixed)
Omit lazy-loading attribute on first content image/iframe
Reported by: | flixos90 | Owned by: | flixos90 |
---|---|---|---|
Milestone: | 5.9 | Priority: | normal |
Severity: | normal | Version: | 5.5 |
Component: | Media | Keywords: | has-patch has-unit-tests has-dev-note |
Focuses: | performance | Cc: |
Description
Per this web.dev article, lazy-loading elements above the fold can lead to slight regressions in the Largest Contentful Paint metric. Since WordPress core by default lazy-loads every image and iframe, this also includes those in the initial viewport which has shown to impact LCP in the field at scale. After analyzing the situation of the current core implementation and the theme ecosystem (see this post), it is worth refining the default behavior of WordPress, in order to provide a solid default lazy-loading experience for the most common layouts (e.g. single column, hero image).
Related: #50425
Change History (54)
#2
@
3 years ago
Just because of thinking, the various website I manage with different themes and plugins.
I think that the best idea should be something like a global that track how many executions of (as example) wp_get_attachment_image
and on first element doesn't add that parameter.
With adding a filter to disable that behaviour.
I know that a global is not the best thing but in this way is more granular in all the website without asking every developer from blocks to theme developers.
#3
follow-up:
↓ 6
@
3 years ago
@Mte90 A global counter is what I was thinking too. I also agree this should be filterable - I'd argue in addition to turning it off it would be useful for themes with specific layouts where more than the first image typically appears above the fold.
@westonruter This is close to what I did in the prototype plugin used for the analysis - the one thing missing from yours (as you mention in the other ticket) was archive support. It would be great if you could review the prototype plugin (see below) and leave your thoughts.
For reference, this Gist has the prototype plugin with the logic used during the analysis:
- Increase a counter for every content image (covering featured images and post content images).
- If the first image is encountered, prevent the
loading="lazy"
attribute from being added. - This works for both single post views and archive views - in an archive, only the first image from the first post will not be lazy-loaded.
This plugin uses a function parameter to control how many images to omit from being lazy-loaded. For the core implementation, we could e.g. introduce a filter to allow modification. A theme that e.g. displays a grid archive with 3 columns could then set the number of images to not lazy-load for archives to 3.
Another thing to note here is that the plugin only focuses on images. The core implementation should also count iframes in the same way, in a common counter - i.e. the first image or iframe won't be lazy-loaded.
#4
@
3 years ago
So for iframe I think that we can do it for oembed (and blocks) but not for custom added inside the posts but should be enough I guess.
Usually a lot of plugins creates widget with iframe like mailchimp or instagram but usually they are used in the footer, so if we cover the oembed I think that is a acceptable percentage of usage.
#5
@
3 years ago
- Milestone changed from Awaiting Review to 5.9
lazy-loading elements above the fold can lead to slight regressions in the Largest Contentful Paint metric
Sounds like something that needs to be fixed in the web browsers? They seem to be in a much better position to "see" (or guess) when an image or an iframe will be above the fold. Having said that, WP can also try to improve the situation until this is fixed in the browsers.
For reference, this Gist has the prototype plugin
...
For the core implementation, we could e.g. introduce a filter to allow modification.
Right. The Gist looks good but seems the code in core will be quite different once implemented.
Usually a lot of plugins create widget with iframes...
Yes, but thinking these plugins should be adding the loading
attribute to their HTML themselves. Such plugins can usually detect when a widget is at the top, in the sidebar, or in the footer.
Looking at FSE/template parts, can probably use the same approach as with widgets: while editing add loading="eager"
when the image or iframe is expected to be at the top/above the fold, and loading="lazy"
when in the middle of the page or in the footer.
Lets look at this during 5.9.
#6
in reply to:
↑ 3
;
follow-up:
↓ 8
@
3 years ago
Replying to flixos90:
@westonruter This is close to what I did in the prototype plugin used for the analysis - the one thing missing from yours (as you mention in the other ticket) was archive support. It would be great if you could review the prototype plugin (see below) and leave your thoughts.
Actually, my plugin does account for the archive template. My comment was referring to how twentytwentyone was not omitting loading=lazy
from the first post's featured image on an archive template.
In your POC, you're stripping out loading="lazy"
from the HTML of the featured image. Would it not be better to rather filter wp_lazy_loading_enabled
? See alternative.
It's good how you're using wp_img_tag_add_loading_attr
to account for markup in the content beyond just the featured image.
What if someone is adding an image to the template outside of the_content()
and the_post_thumbnail()
? They could also obtain an image via wp_get_attachment_image()
directly. Since that function applies the wp_lazy_loading_enabled
filter, perhaps the filter should increment the counter instead of in the post_thumbnail_html
and wp_img_tag_add_loading_attr
filters both?
This ticket was mentioned in PR #1580 on WordPress/wordpress-develop by felixarntz.
3 years ago
#7
- Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed
This PR implements the refinement described in this post as a core enhancement.
- Introduce a new function
wp_get_loading_attr_default( $context )
that should be used in place of the hard-coded 'lazy' string defaults used throughout the codebase so far.- The function still returns 'lazy' in most circumstances.
- However, for contexts
the_content
andthe_post_thumbnail
(new, see below) it returnsfalse
(to omit theloading
attribute) for the very first image or iframe within the main query loop.
- Introduce a new filter
wp_omit_loading_attr_threshold
in order to allow fine tuning of the default threshold of 1 content image or iframe (e.g. based on special layout usage by a theme). For example, if the filter is used to set the threshold to 3, the first 3 images or iframes won't be lazy-loaded. - A new context
the_post_thumbnail
is introduced, used withinget_the_post_thumbnail()
and thus taking precedence over thewp_get_attachment_image
context that would be used bywp_get_attachment_image()
. - One rather minor, but significant change has been made to
wp_filter_content_tags()
:- The function used to first go over all images and then all iframes.
- However, since now sequence truly matters in order to decide whether or not to add a
loading
attribute, the function now goes over all the matches in their correct order. Nothing else has changed in the logic there, and no existing tests are affected by this change.
- Test coverage is present.
Trac ticket: https://core.trac.wordpress.org/ticket/53675
#8
in reply to:
↑ 6
@
3 years ago
@azaozz @westonruter Would you mind reviewing / testing the PR https://github.com/WordPress/wordpress-develop/pull/1580?
Replying to westonruter:
What if someone is adding an image to the template outside of
the_content()
andthe_post_thumbnail()
? They could also obtain an image viawp_get_attachment_image()
directly. Since that function applies thewp_lazy_loading_enabled
filter, perhaps the filter should increment the counter instead of in thepost_thumbnail_html
andwp_img_tag_add_loading_attr
filters both?
This question may now be slightly off in relation to the core implementation, however I wanna address the point of considering manually placed images via wp_get_attachment_image()
. It may be worth thinking about this, but so far I'm thinking that the use-case of manually rendered images within the content is too insignificant, plus themes may use this to e.g. render small icon-like images etc., considering which here would be rather counter-productive. Of course the post content could also contain such tiny icon-like images as the first elements, but I think that's a lot less likely.
I think relying on only featured images and in-content images is more reliable here.
3 years ago
#9
@felixarntz have you thought about how we could have more precise heuristics going forwards that can take the semantics and structure of blocks into account to get a sense for what is actually deferrable? For example, an image block or a site logo used in a header template part would be strong indicatives of being above the fold. "The first image of the content" seems instead like a rudimentary measure, that varies a lot depending on preceding layout. With block themes we should have some ahead-of-time awareness of layout which we can use to produce more meaningful instructions.
felixarntz commented on PR #1580:
3 years ago
#10
@mtias Great point, there is definitely room for further refinement as we're moving towards a world of block themes. I've been thinking about detecting the header template part as well.
The analysis I've conducted using the fix here (even though technically I agree that "the first content image" _sounds_ rudimentary) has proven that it already significantly improves performance across the board, so I think it's a useful enhancement on its own - I don't think we should overcomplicate it for now. Once there are more block-based themes available, I agree it would make sense to refine further by using the semantics of blocks. For example, we could also make decisions like "if the first block is a gallery block with three images per row, omit lazy-loading on the first _three_ images".
westonruter commented on PR #1580:
3 years ago
#11
Also wanted to note that I found that 3,801 themes (out of 4,145) make use of the main
element. I was surprised (and heartened) to see that 90%+ of themes use this semantic element. For the AMP plugin, we've used this as an easy way to identify all images that appear in the header, by looking for all that occur before the `main`. Granted, this wouldn't work the same way in core since the DOM of the page is not available, but eventually in a FSE context this could be implemented similarly through filtering of blocks.
3 years ago
#12
@felixarntz yes, I didn't mean rudimentary in a disparaging way, just that it feels we could be doing more tailored targeting there with block semantics.
adamsilverstein commented on PR #1580:
3 years ago
#13
adamsilverstein commented on PR #1580:
3 years ago
#14
verified working correctly in twentytwenty
so must be something specific about how 2019 outputs that top image.
felixarntz commented on PR #1580:
3 years ago
#15
@adamsilverstein
Testing with
twentynineteen
, the header/top image seems to still get the lazy attribute, while for content's first image which you have to scroll to reach does not get it.
That was expected behavior with the proposed fix, since that theme is rendering the featured image outside of the loop. It raises a good point though that maybe we should support that case, since it is fairly common to render the featured image on top of the page when is_singular()
. I've implemented this in https://github.com/WordPress/wordpress-develop/pull/1580/commits/03a95a93ecb0b267533e8bfb5825e690541fe152. Similar to the other changes in this PR, this is not 100% reliable, but it won't have adverse effects, and I'm assuming it will bring even better LCP results than without it.
I will run a separate follow-up A/B test to compare. Until then, happy to hear your feedback @westonruter on this new commit as well.
felixarntz commented on PR #1580:
3 years ago
#16
@adamsilverstein @westonruter @thierryA I've ran a test between both versions of the fix (see above https://github.com/WordPress/wordpress-develop/pull/1580#issuecomment-931592700) across the 30 most "popular" themes based on the wordpress.org API. To clarify, the two different versions are:
- "corefix1" counts all
the_content
andthe_post_thumbnail
images as long as they're alsoin_the_loop()
. - "corefix2" is based on the latest iteration here, so it supports one extra condition where
the_post_thumbnail
is also counted for a "singular" context, even if notin_the_loop()
(where it e.g. covers the special behavior in Twenty Nineteen).
The difference between the two is barely noticeable:
- For both LCP and image KB loaded, the median difference is 0%.
- Especially for LCP there are no notable wins on either end, although the number of tests where there is an improvement in "corefix2" is slightly higher than the number of tests with an improvement in "corefix1". That isn't statistically sufficient for a decision though.
- In image KB, there is barely any difference, in most individual tests the difference is 0%. It's in fact only 4 themes where any scenario gave a result other than 4%, and two of those are indeed Twenty Nineteen and Twenty Seventeen, which both for a singular post have that special behavior that they display the featured image outside of the loop.
Given that there is no win across the board from also counting featured images in singular context outside the loop and since it is also a special case anyway, I suggest we revert https://github.com/WordPress/wordpress-develop/pull/1580/commits/03a95a93ecb0b267533e8bfb5825e690541fe152 again and go with the original approach. I don't think adding a special case just for some core themes with a non-standard implementation is justifiable. Thoughts?
adamsilverstein commented on PR #1580:
3 years ago
#17
Given that there is no win across the board from also counting featured images in singular context outside the loop and since it is also a special case anyway, I suggest we revert 03a95a9 again and go with the original approach. I don't think adding a special case just for some core themes with a non-standard implementation is justifiable. Thoughts?
I agree your code here should be as generalized as possible and you should revert the final change. One thing to consider would be changing twentynineteen itself since we maintain that in core as well and I'm guessing it is still in very wide usage.
felixarntz commented on PR #1580:
3 years ago
#18
@adamsilverstein Reverted!
We can look at changing Twenty Nineteen as well, but it might be tricky. While it should be possible to move that header content into the loop without affecting the actual design, a problem would be child themes. For example a child theme that overrides header.php
and nothing else would then most likely see that image twice because they would still use the template based on the old version of the theme.
3 years ago
#19
Given that there is no win across the board from also counting featured images in singular context outside the loop and since it is also a special case anyway, I suggest we revert 03a95a9 again and go with the original approach. I don't think adding a special case just for some core themes with a non-standard implementation is justifiable. Thoughts?
Sounds totally reasonable to me, +1!
3 years ago
#20
I'm actually having some "second thoughts" on this. As far as I understand this is a browser problem. The browsers should be able to detect when an image s going to be in the viewport and load it eagerly. That's in the specs.
It is somewhat unfortunate that this doesn't currently work particularly well as seen by @felixarntz's post, but I'm not 100% sure WordPress should be trying to partially remedy the situation. No matter what we do it will always be a partial, hit-and-miss solution as there's no chance to guess the viewpors from the server. The browsers are much better equipped for that and should be fixing it.
Another thing is that WordPress already slows things down pretty considerably by implementing srcset
and sizes
and loading
attributes on-the-fly at the front-end. Making many billions of page loads on many millions of sites even a bit slower is a hefty price to pay for a somewhat partial improvement.
Thinking it would be better to see if the browsers fix this and in case they don't/won't perhaps try to add some code on the server. My suggestion would be to come back to this in 6 months to a year and asses it again.
felixarntz commented on PR #1580:
3 years ago
#21
@azaozz
I'm actually having some "second thoughts" on this. As far as I understand this is a browser problem. The browsers should be able to detect when an image is going to be in the viewport and load it eagerly. That's in the specs.
The problem is that the browser is only able to detect that when the layout has been computed. This is why there is a slight delay on lazy-loaded images at the top - the browser still today loads the top images as soon as it knows they're at the top, but that is after the layout has been computed. Waiting for this layout computation is what causes the LCP regression I outlined in the post. However, we shouldn't just disable lazy-loading either, since then _all_ images would loaded right away, which is obviously bad.
No matter what we do it will always be a partial, hit-and-miss solution as there's no chance to guess the viewport from the server. The browsers are much better equipped for that and should be fixing it.
See above, the browsers aren't particularly better equipped unfortunately due to the layout computation being required. I used to believe the same thing that you're saying, but it is unfortunately not the case. I agree a partial fix isn't perfect, but it is still a partial fix, and the analysis has shown that it brings a notable performance improvement in most cases. The performance improvement things brings on the client side is vastly more relevant than a tiny bit of extra logic being run on the server (which is not particularly expensive).
Thinking it would be better to see if the browsers fix this and in case they don't/won't perhaps try to add some code on the server. My suggestion would be to come back to this in 6 months to a year and asses it again.
I will keep pushing and we can see if at some point the browser can improve some heuristics for this, but the browsers are limited in fixing this problem per my explanation above. The enhancement here is entirely non-disruptive, there is no API change, so it could even be removed again at some point, should a better solution arise. This isn't going to be the case anytime in the not-so-distant future though. We should proactively improve performance of WordPress, not hope that somebody else will do it for us.
westonruter commented on PR #1580:
3 years ago
#22
Right, browsers are inherently limited in that they cannot foresee the future. It's impossible for them to know whether to start loading a lazily-loaded image prior to layout since they won't know if it is going to be in the viewport. WordPress can help the browser out by making educated guesses as to which images are going to be in the first viewport so that the browser doesn't have to wait for layout to occur.
jono-alderson commented on PR #1580:
3 years ago
#23
Seconding; in an ideal world, the browser would handle this. But it can't, until it's too late for it to be beneficial.
Note that this is also going to start creating warnings for everybody who's checking their site in Lighthouse: https://developer.chrome.com/blog/new-in-devtools-95/#lighthouse
3 years ago
#24
I am in favor of prioritizing the end user experience vs the ideal implementation details (without ignoring its cost indeed).
And the analysis has shown that it brings a notable performance improvement in most cases. The performance improvement things brings on the client side is vastly more relevant than a tiny bit of extra logic being run on the server (which is not particularly expensive).
There is a clear improvement in most cases.
The enhancement here is entirely non-disruptive, there is no API change, so it could even be removed again at some point, should a better solution arise.
While the implementation is not _perfect_, its cost is very low and non disruptive.
To me it is a clear cut, the pros outgrows the cons.
3 years ago
#25
I am in favor of prioritizing the end user experience vs the ideal implementation details (without ignoring its cost indeed).
And the analysis has shown that it brings a notable performance improvement in most cases. The performance improvement things brings on the client side is vastly more relevant than a tiny bit of extra logic being run on the server (which is not particularly expensive).
There is a clear improvement in most cases.
The enhancement here is entirely non-disruptive, there is no API change, so it could even be removed again at some point, should a better solution arise.
While the implementation is not _perfect_, its cost is very low and non disruptive.
To me it is a clear cut, the pros outgrows the cons.
This ticket was mentioned in Slack in #core-media by antpb. View the logs.
3 years ago
3 years ago
#27
@felixarntz, @westonruter, @jono-alderson, @ThierryA I see. So the browsers "cannot" determine fast enough whether an image would be in the viewport, and do not try to optimize this? That's kind of unfortunate :)
Even in this case the browsers are better equipped to estimate when an image may be in the viewport and load it eagerly, as they "see" all of the HTML. WordPress can "see" only part of the HTML from the server excluding any images that may be in the header, sidebar(s), etc. so any estimates made on the server will always be inferior to estimates made by the browsers.
In addition it seems that the browsers do not want to do the same optimization as proposed here for some reason. Yeah, I believe it can be done there better.
Anyways, if this is going to be added as a "partial enhancement", thinking the most important part would be to be as fast as possible, even if that means making it a bit less effective. Looking at the current patch there seem to be few places this can be speed up a but. For example there's a proposed filter wp_omit_loading_attr_threshold
that should be running only once per page load, not for every image (filters can sometimes introduce large slow-downs). Also perhaps the global $wp_content_media_count
should be a static instead? I'll comment inline too.
westonruter commented on PR #1580:
3 years ago
#28
So the browsers cannot determine fast enough whether an image would be in the viewport, and do not try to optimize this? That's kind of unfortunate :) […] In addition it seems that the browsers do not want to do the same optimization as proposed here for some reason. Yeah, I believe it can be done there better.
I don't think it's a matter of browsers not wanting to do better. I know Chromium at least is goes to extraordinary lengths to make every performance gain possible. Even with the HTML in hand, a browser wouldn't be able to quickly determine whether a given image is going to be in the first viewport. There could be a header
that pushes images out of the initial viewport once the page is laid out, for example. Or there could be a lot of content in the header (e.g. huge nav menu) which browsers may think implies an image way later won't be in the first viewport, when in reality most of the nav menu is hidden. So the only way for the browser to know whether to start loading an image is to do a layout, even if it is a layout guess based on some HTML heuristics. Ultimately this would cause more latency for rendering the page, however. In contrast, when WordPress is rendering the page it has a much better idea for whether an image is going to be in the first viewport based on its knowledge of how pages are constructed. So it makes sense to me that it should be WordPress's responsibility to provide the hints to the browser by omitting the loading=lazy
attributes for images in the initial viewport.
3 years ago
#29
@westonruter:
Even with the HTML in hand, a browser wouldn't be able to quickly determine whether a given image is going to be in the first viewport.
Right, then the trade-off would be that one or two images may eventually be loaded eagerly instead of lazily in some (rare?) cases, right? Considering the benefits as described by @felixarntz in https://make.wordpress.org/core/2021/07/15/refining-wordpress-cores-lazy-loading-implementation/, thinking the tradeoff is perhaps worth it? If not, why are we adding this enhancement to WP :)
when WordPress is rendering the page it has a much better idea for whether an image is going to be in the first viewport based on its knowledge of how pages are constructed.
Hmm, hot sure about this. WP core (on the server) has no idea if there are other images that were outputted before the first image it sees, and what the theme's layout might be. It's just a guess. It also has no idea of the browser window size and shape, screen size, device, etc. which also affect the viewport. So yes, seems the browsers have some more data to make that guess more accurately.
As far as I see it: If it's worth doing in WordPress with a lot less data to make a good estimate, it's probably worth doing in the browsers too" :)
westonruter commented on PR #1580:
3 years ago
#30
WP core (on the server) has no idea if there are other images that were outputted before the first image it sees, and what the theme's layout might be.
@azaozz WordPress has standard theme features like the header image, custom logo, featured image which are located in the first viewport by the overwhelming number of themes. So the guesses are highly educated guesses, certainly accounting for >80% of themes.
3 years ago
#31
WordPress has standard theme features like...
...
So the guesses are highly educated guesses
Right, perhaps in some cases, but "totally off" in other. Yes, WordPress has some more or less standardized features that usually would output images in the first viewport, but these features often output more than one image there, or there may be images outputted directly by the theme's HTML that cannot be "seen" by core, etc.
Anyway, my point is that it will always be more difficult to guess from the server what would happen in the browser. It would always be easier to guess from the browser what may be in the initial viewport after content is fully rendered.
felixarntz commented on PR #1580:
3 years ago
#32
@azaozz If we make these variables static, there still has to be a way to reset them. There is no way to make WP's function-based code 100% bulletproof to other developers - if they want to misuse/break it they can already in a million ways.
Your latest commit broke the tests because now the filter isn't re-triggered for new tests. So again, I don't disagree with the approach of making it static, but it doesn't work _just_ like that. There has to be a way to "reset" the filter. I suggest we put it into a function like wp_omit_loading_attr_threshold( $force = false )
, with the $force
parameter allowing to enforce re-running it, should the value change, which is necessary for tests to work.
3 years ago
#33
If we make these variables static, there still has to be a way to reset them.
Hmm, yes but, why? We have to change the functionality just for the sake of testing? Shouldn't be the other way round -- make this work the best way possible then add/fix/tweak the tests to work with the code :)
Your latest commit broke the tests because...
Yeah, was looking at how to "suggest" changes, i.e. make a variant of the existing patch. However that seems impossible on GH. The only way would be to make another PR which is not good in this case.
Basically I'm thinking that both changes should be made inside wp_get_loading_attr_default()
. Then it won't need yet another function like wp_increase_content_media_count()
which doesn't do anything in production and even could be misused, eventually. The only thing stopping this now are the tests that need to be made to work with the changed code.
felixarntz commented on PR #1580:
3 years ago
#34
@azaozz
Hmm, yes but, why? We have to change the functionality just for the sake of testing? Shouldn't be the other way round -- make this work the best way possible then add/fix/tweak the tests to work with the code.
I overall agree with that statement. However, code still has to be written in a testable way if we want to add tests for it - and having state which cannot be controlled in tests prevents that (at a minimum we need to allow reset between tests). We simply cannot "lock" the code so that nobody can override it, as otherwise we also lock it for tests. What we _can_ do is define solid patterns to make it less likely to happen "by accident", and to your point the approach with static variables and dedicated functions makes the access more intentional than just having a global.
Basically I'm thinking that both changes should be made inside
wp_get_loading_attr_default()
. Then it won't need yet another function likewp_increase_content_media_count()
which doesn't do anything in production and even could be misused, eventually, because it can set or reset that count instead of using the filter. The only thing stopping this now are the tests that need to be made to work with the changed code.
See above, I don't think there's a way to make this entirely "locked" static code testable without reducing our test coverage. The reason most WP core tests work out of the box without any of this extra "magic" is that core uses globals, and those can always easily be overwritten, and they automatically get reset between tests. Anywhere where we don't use globals, a special mechanism is needed.
I'll look at changing/fixing the tests later today and propose another change (well, commit another change as per the above). Then we can see/decide :)
Curious what you come up with. I'll for now update the PR based on your latest commit to make this static mutable again to "unlock" it.
With all that said, the discussion we're having here talks about general code and test patterns in WP core, so I'm not sure this issue is the proper place to have it. Core uses globals throughout, so from that perspective I'd even be in favor of sticking with the approach. If we change this here, it doesn't mean anything, the rest of the codebase will still follow the old pattern, and almost no other core developers will be aware of the new pattern unless it is explicitly called out e.g. in coding standards documentation - it could even be that some folks don't agree with it, another reason not to silo this wide-reaching topic discussion in a very specific PR like this.
hellofromtonya commented on PR #1580:
3 years ago
#35
Another option for making memoization testable is to move the code into a class and making the memoization value a property. If the class is used as a singleton, the static is not needed; else, it is. That's a design choice. Tests can access the property via reflection or by instantiating a new instance for each test.
Advantages to this approach are:
- the avoidance of added test related code into the production code
- ability to break up larger functions into smaller tasks with private/protected methods
3 years ago
#36
code still has to be written in a testable way...
Right, the question here is what are we testing. As the wp_omit_loading_attr_threshold
filter has to run only once per page load (see above), we should probably be testing that (if it makes sense), not running it multiple times which is not supposed to happen.
We simply cannot "lock" the code so that nobody can override it
Hm, we are not "locking" the code, think of $content_media_count
as a private property of a class. Functionality is pretty similar.
The current code as of https://github.com/WordPress/wordpress-develop/pull/1580/commits/1c218d7390ea0fb9ea91939bf7db800c842bb1f2 does not achieve these objectives:
- The
wp_omit_loading_attr_threshold
filter can be run more than once by using the just introducedwp_omit_loading_attr_threshold( $force = false )
function. - The
wp_increase_content_media_count( $amount = 1 )
can be used to arbitrarily reset the$content_media_count
which should not be re-settable/changeable at all.
With all that said, the discussion we're having here talks about general code and test patterns in WP core...
Hmm, not sure these are "new" things. It's best if tests cover the intended usage, that has been the case for a long time. The current/old tests do not follow the intended usage exactly any more, so they will need adjusting.
The need to change the tests arises from making some changes to the code, namely the wp_omit_loading_attr_threshold
filter has to run only once per page load, and the $content_media_count
variable has to be "private" or non-changeable (and it's pretty bad idea to introduce a PHP global at this point).
...move the code into a class and making the memoization value a property.
Yes, this would have worked if that code was in a class/wrapper. However changing this is a major refactoring considering back-compat, etc. It may happen one day, but not for 5.9. This patch is a relatively small correction/enhancement to how WordPress adds the loading
HTML attribute for images and iframes.
felixarntz commented on PR #1580:
3 years ago
#37
@azaozz
we should probably be testing that (if it makes sense), not running it multiple times which is not supposed to happen.
Agreed, we should be testing that as well, this requires adding one more test.
Hm, we are not "locking" the code, think of
$content_media_count
as a private property of a class. Functionality is pretty similar.
The current code as of https://github.com/WordPress/wordpress-develop/pull/1580/commits/1c218d7390ea0fb9ea91939bf7db800c842bb1f2 does not achieve these objectives:
- The
wp_omit_loading_attr_threshold
filter can be run more than once by using the just introducedwp_omit_loading_attr_threshold( $force = false )
function.- The
wp_increase_content_media_count( $amount = 1 )
can be used to arbitrarily reset the$content_media_count
which should not be re-settable/changeable at all. For example: think of a theme or plugin doing something likewp_increase_content_media_count( 1000 )
, i.e. disabling the whole thing in the wrong way.
See my previous comment. What you are proposing here makes this code untestable, except if the WP core test suite was starting a new PHP process for every single test which... would make the tests take hours, if not days. I understand this is similar to a private property of a class - to @hellofromtonya's point, that approach would be an appropriate solution, and this is how most sophisticated PHP projects do it. Still, I agree this is too much of a refactoring which this PR is not the place for.
The limitation in the approach you're proposing is that the point of only running once per page-load only applies to that, a page-load. However, PHPUnit tests aren't like a page load, they run tons of things that would normally happen through tons of page loads in a single run.
Hmm, not sure these are "new" things. It's best if tests cover the intended usage, that has been the case for a long time. The current/old tests do not follow the intended usage exactly any more, so they will need adjusting.
This statement confirms to me that this is not the right place to make this change. We are speaking about a change in coding paradigm that requires refactoring a ton of existing tests and also the related production code - basically "remove globals and manage state elsewhere in a testable way", which is surely not something we can or should arbitrarily decide in an unrelated PR.
3 years ago
#38
What you are proposing here makes this code untestable
Hmmm, not exactly? Lets look at the tests. The problem with the test for the wp_omit_loading_attr_threshold
is that it is being initialized more than once with different values. However I don't see what is being tested by initializing it multiple times. Yeah, I understand that using static
in functions may not be a very well used method, but it seems simplest and fastest. Feel free to implement that in a different way.
Same for the $content_media_count
variable. The current/old tests were written to test a global variable. However it was established that it should not be a global variable and the code was changed. So the tests will have to be changed to account for that. A good way would be if the tests do not access that variable at all, as that's how the code is supposed to work now.
This statement confirms to me that this is not the right place to make this change. We are speaking about a change in coding paradigm that requires refactoring a ton of existing tests...
Uh, sorry but I don't seem to understand exactly what you want to say. What's happening with this patch is: you asked for a review, I did a review, this review established that some changes are needed, the changes were implemented. What's left is to remove the old tests that were written with the old code in mind, and to add new tests that cover the new code. What am I missing?
felixarntz commented on PR #1580:
3 years ago
#39
@azaozz
Hmmm, not exactly? Lets look at the tests. The problem with the test for the
wp_omit_loading_attr_threshold
is that it is being initialized more than once with different values. However I don't see what is being tested by initializing it multiple times.
The filter being applied as expected is being tested. If we can only call it once, we are unable to test that. We could remove that test, but then the production code function could literally be function() { return 1; }
(i.e. without applying the filter), and the remaining tests wouldn't catch that.
Uh, sorry but I don't seem to understand exactly what you want to say. What's happening with this patch is: you asked for a review, I did a review, this review established that some changes are needed, the changes were implemented. What's left is to remove the old tests that were written with the old code in mind, and to add new tests that cover the new code. What am I missing?
The tests were already updated to fit the new code, they still test that the filter is applied correctly, and they still pass.
3 years ago
#40
The filter being applied as expected is being tested. If we can only call it once, we are unable to test that.
Ummm, you mean for a filter to be tested is has to run multiple times? Why? We know the default value and we know the filtered value. running the filter is supposed to change the default to the filtered value. What's the point in running the same test again?
The tests were already updated to fit the new code..
If I'm not mistaken that update should have included a change to exclude use of the (now supposedly "private") $content_media_count
variable. Again, may be missing something here too...
felixarntz commented on PR #1580:
3 years ago
#41
Ummm, you mean for a filter to be tested is has to run multiple times? Why? We know the default value and we know the filtered value. Running the filter is supposed to change the default to the filtered value. What's the point in running the same test again?
Technically, if we only run the filter once (e.g. with returning 3
), it could also be that the production function has no filter at all and it just returns 3
hard-coded. This is arguably a bit of nit-picking, but there is another more significant problem: Even if we only ran the filter once in that tests, we would still need to run it again in one of the other tests. If we in any test use the filter to set the number to 3
, it will be 3
throughout all tests, even the ones where we want the default value of the filter to be used. For the content media count, this is even more problematic: There is more than one test that needs to use this functionality, but for any test that is not the very first one being run, it will never start at a count of 0 (since the other test already increased the count). This is why running the filter as well as the content media count variable have to be reset between tests.
There is surely _some_ way to make the tests all pass without a reset, but it would make them hard to understand as well unstable (e.g. what if they run in a different order), since it would pollute the overall state which is a big no-go in tests. Global state needs to be reset between tests so that we mirror production behavior, i.e. ensuring the filter runs once per test which is the test "equivalent" of per page load.
Today, after every core test, all globals are reset, all filters and actions are removed, etc. We have to ensure here as well that the state modified via one test doesn't affect another, that's why a simple static
without a way to reset won't do it.
3 years ago
#43
There is surely some way to make the tests all pass without a reset, but it would make them hard to understand...
Right, testing "static caching" is a bit different and somewhat harder. Think of it as testing PHP constants: they need to be tested if they are set, and are set to the proper values, but no other tests are needed as they can never be changed later. So generally they require less testing that variables, etc.
Today, after every core test, all globals are reset, all filters and actions are removed, etc.
Yes, and the tests here do exactly the same: there are no globals, and the filter is removed. However there is a constant that is set by WordPress when using this functionality. It cannot be reset which is the proper behavior as that's exactly what happens in production.
In any case, you're an experienced developer and know this part of the code very well. We worked really well together when implementing the main functionality. This is a relatively small (by code size) enhancement. Lets add it as-is if you think it's fine.
felixarntz commented on PR #1580:
3 years ago
#44
@azaozz It's a good point to bring up the comparison with constants, particularly for the filter which shouldn't change its value on page load. The thing with those though is that they're typically defined early on at runtime (e.g. in wp-config.php
, if a constant is defined within a "random" function that needs to be tested, it would make adding test coverage for it very challenging, if not impossible as well.
That said, it briefly made me think about whether we should actually have a constant here instead of a filter. That probably wouldn't be flexible enough though since the filter may be used in different ways e.g. based on which template is currently being loaded, which we wouldn't be able to do with a constant.
Thanks for trusting my judgement here. We're indeed nit-picking about a rather minor part of the functionality, so we can probably move this forward.
This ticket was mentioned in Slack in #core-media by antpb. View the logs.
3 years ago
This ticket was mentioned in Slack in #core-media by antpb. View the logs.
3 years ago
3 years ago
#47
That said, it briefly made me think about whether we should actually have a constant here instead of a filter. That probably wouldn't be flexible enough though..
Right, thinking a filter is a lot better there, as it is now.
We're indeed nit-picking about a rather minor part of the functionality, so we can probably move this forward.
Exactly right 👍
felixarntz commented on PR #1580:
3 years ago
#50
Committed in https://core.trac.wordpress.org/changeset/52065.
I have a POC patch in plugin form linked to at https://core.trac.wordpress.org/ticket/50425#comment:14