wp-media / wp-rocket

Performance optimization plugin for WordPress
https://wp-rocket.me
GNU General Public License v2.0
689 stars 216 forks source link

3.16 Preload - Shouldn't fetch RSS feed or restAPI links from the Homepage #6575

Closed hanna-meda closed 4 months ago

hanna-meda commented 5 months ago

Before submitting an issue please check that you’ve completed the following steps:

Describe the bug Currently, having internal RSS feed or restAPI links between the first 10 links on the Homepage, they will be fetched.

To Reproduce Steps to reproduce the behavior:

  1. On new.rocketlabsqa.ovh set "homepage_with_rss_feed_restAPI_links" as Homepage.
  2. Clear cache to trigger link fetching.
  3. Check the URLs inside "Add to queue request arguments" logs in wp-rocket-debug.log.html.
  4. Notice that between the links fetched are also RSS feed and restAPI links, i.e:

    https://new.rocketlabsqa.ovh/feed/ https://new.rocketlabsqa.ovh/wp-json/wp/v2/users

Expected behavior RSS feed and restAPI links should be excluded from fetching.

Screenshots

Screenshot 2024-04-18 at 21 32 02 Screenshot 2024-04-18 at 21 31 42

Additional context Add any other context about the problem here.

Acceptance Criteria (for WP Media team use only) Clear instructions for developers, to be added before the grooming

Tabrisrp commented 5 months ago

Scope a solution ✅

Engine\Media\AboveTheFold\WarmUp\Controller

Estimate the effort ✅

Effort [S]

Miraeld commented 5 months ago

Looks good to me

hanna-meda commented 5 months ago

Related TCs 41737 & 41744.

MathieuLamiot commented 5 months ago

@jeawhanlee I still get the same issue when testing this branch on new.rocketlabs. Did you check that this PR is doing what is expected?

image

(There is also the wp-json link there).

I think the test case you added in the fixture is wrong because the wp-json and feed links you added have the new.rocketlabs host. So they are discarded in the fetch_links method as they are external links maybe? You should try to put the right hostname in the fixture maybe? If this is correct, as a way to improve and avoid this type of mistake from now on, the TDD approach would have helped: adding the new fixture first, you would have seen that the test was still passing. This would allow you to identif the fixture is not doing what you expect from it?

jeawhanlee commented 5 months ago

@MathieuLamiot Thanks for pointing this out, indeed the fixture contained the wrong host, As a matter of fact, the TDD approach was used here and the test failed initially, but along the line, I recopied the wrong fixture from an online html minifier.