Fallback value for src attribute if not set #6700

MathieuLamiot · 2024-06-05T18:03:24Z

Remove duplicated code
Fallback value for src attribute if not set

Description

Fixes #6699

Documentation

User documentation

Prevents Fatal error on LCP/ATF optimization of picture and img-srcset

Technical documentation

Remove duplicated code
Fallback value for src attribute if not set

Type of change

Delete options that are not relevant.

New feature (non-breaking change which adds functionality).
Bug fix (non-breaking change which fixes an issue).
Enhancement (non-breaking change which improves an existing functionality).
Breaking change (fix or feature that would cause existing functionality to not work as before).

New dependencies

NA

Risks

NA

Checklists

Feature validation

I validated all the Acceptance Criteria. If possible, provide sreenshots or videos.
I triggered all changed lines of code at least once without new errors/warnings/notices.
I implemented built-in tests to cover the new/changed code.

Documentation

I prepared the user documentation for the feature/enhancement and shared it in the PR or the GitHub issue.
The user documentation covers new/changed entry points (endpoints, WP hooks, configuration files, ...).
I prepared the technical documentation if needed, and shared it in the PR or the GitHub issue.

Code style

I wrote self-explanatory code about what it does.
I wrote comments to explain why it does it.
I named variables and functions explicitely.
I protected entry points against unexpected inputs.
I did not introduce unecessary complexity.
I listed the introduced external dependencies explicitely on the PR.
I validated the repo-specific guidelines from CONTRIBUTING.md.

Observability

I handled errors when needed.
I wrote user-facing messages that are understandable and provide actionable feedbacks.
I prepared ways to observe the implemented system (logs, data, etc.).

Risks

I explicitely mentioned performance risks in the PR.
I explicitely mentioned security risks in the PR.

Fallback value for src attribute if not set

MathieuLamiot · 2024-06-05T18:29:13Z

This PR solves the reported issue on the template shared in the GH issue: No more fatal error and I can see the preload applied as on 3.16:

<link rel="preload" as="image" href="http://mathieu.e2e.rocketlabsqa.ovh/wp-content/rocket-test-data/images/lcp/testavif.avif" media="(min-width: 700px)" fetchpriority="high">
<link rel="preload" as="image" href="http://mathieu.e2e.rocketlabsqa.ovh/wp-content/rocket-test-data/images/lcp/testPng.png" media="(min-width: 450px)" fetchpriority="high">

Two tests are failing for the following reasons, in create_object:

With @Khadreal latest changes, we would set both src and bg_set in case both exist in the $image object. src because it was manually added outside the switch/case, and bg_set as the first attribute in the foreach loop.
Before, we would only set the first non-empty attribute. As specified in the comment by @Tabrisrp: For other types, add the first non-empty key to the object.

@Tabrisrp @Khadreal Can you confirm if this is a wanted change to populate both now? I feel like this is just something that was missed and that we don't need to populate both. In which case, the tests are wrong and should be adapted. Otherwise, we just need to re-add the part I removed:

		if ( is_array( $image->src ) ) {
			$sanitized_object_array = array_map(
				function ( $item ) {
					if ( ! empty( $item->src ) ) {
						$item->src = $this->sanitize_image_url( $item->src );
					}
					return $item;
				},
				$image->src
			);

			$object->src = $sanitized_object_array;
		} elseif ( ! empty( $image->src ) ) {
			$object->src = $this->sanitize_image_url( $image->src );
		}

WDYT?

Tabrisrp · 2024-06-05T19:51:49Z

Each array item of a bg_set should have a src property, but the main object doesn't need to have a src property.

I think we're packing too much into the default case, and we could break down this part into cases for the other types depending on their needs.

codacy-production · 2024-06-05T20:31:16Z

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
Report missing for `6fc1a47`¹	✅ 100.00% (target: 50.00%)

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (`6fc1a47`)	Report Missing	Report Missing	Report Missing
Head commit (`7cc4788`)	37258	14429	38.73%

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#6700)	2	2	100.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings Change summary preferences

_{Codacy will stop sending the deprecated coverage status from June 5th, 2024. Learn more}

Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct. ↩

Mai-Saad · 2024-06-06T02:26:05Z

This template http://mathieu.e2e.rocketlabsqa.ovh/lcp_no_dimensions_picture has the following markup
<picture> <source media="(max-width: 700px)" srcset="<?php echo content_url().'/rocket-test-data/images/lcp/testavif.avif' ?>"> <source media="(max-width: 450px)" srcset="<?php echo content_url().'/rocket-test-data/images/lcp/testPng.png' ?>"> <img src='/wp-content/rocket-test-data/images/lcp/testjpg.jpg' alt="GFG" data-width="" > </picture>
which isn't preloaded correctly on dev (leaving <img behind), the problem shouldn't happen on #6659 .. by default any picture shall have at least 1 <img>

MathieuLamiot · 2024-06-06T08:42:02Z

Rocket-E2E results:

This Node.js version (v21.6.2) has not been tested with this version of Cucumber; it should work normally, but please raise an issue if you see anything unexpected.
Error deleting folder "./backstop_data/bitmaps_test": ENOENT: no such file or directory, lstat './backstop_data/bitmaps_test'
Loading config:  /Users/mathieu/Documents/Github/wp-rocket-e2e/backstop.json 

COMMAND | Executing core for "reference"
  clean | backstop_data/bitmaps_reference was cleaned.
createBitmaps | Selected 6 of 6 scenarios.
CREATING NEW REFERENCE FILE
CREATING NEW REFERENCE FILE
CREATING NEW REFERENCE FILE
CREATING NEW REFERENCE FILE
CREATING NEW REFERENCE FILE
Browser Console Log 0: JSHandle:JQMIGRATE: Migrate is installed with logging active, version 3.4.1
Browser Console Log 0: JSHandle:BackstopTools have been installed.
Browser Console Log 0: JSHandle:JQMIGRATE: Migrate is installed with logging active, version 3.4.1
Browser Console Log 0: JSHandle:JQMIGRATE: Migrate is installed with logging active, version 3.4.1
Browser Console Log 0: JSHandle:JQMIGRATE: Migrate is installed with logging active, version 3.4.1
Browser Console Log 0: JSHandle:BackstopTools have been installed.
Browser Console Log 0: JSHandle:BackstopTools have been installed.
Browser Console Log 0: JSHandle:BackstopTools have been installed.
x Close Browser
x Close Browser
x Close Browser
Browser Console Log 0: JSHandle:BackstopTools have been installed.
x Close Browser
CREATING NEW REFERENCE FILE
Browser Console Log 0: JSHandle:JQMIGRATE: Migrate is installed with logging active, version 3.4.1
Browser Console Log 0: JSHandle:BackstopTools have been installed.
x Close Browser
x Close Browser

Run `$ backstop test` to generate diff report.

      COMMAND | Command "reference" successfully executed in [30.972s]
6/25 steps [=================================                                                                                                         ] 0
1) Scenario: When I visited a page that has LCP with relative image # src/features/lcp-beacon-image.feature:10
   ✔ Before # src/support/hooks.ts:132
   ✔ Given I am logged in # src/support/steps/general.ts:23
   ✔ And plugin is installed 'new_release' # src/support/steps/general.ts:29
   ✔ And plugin is activated # src/support/steps/general.ts:48
   ✔ And I go to 'wp-admin/options-general.php?page=wprocket#dashboard' # src/support/steps/general.ts:93
   ✔ When I log out # src/support/steps/general.ts:136
   ✖ Then lcp image in 'lcp_regular_image_template' has fetchpriority # src/support/steps/lcp-beacon-script.ts:111
       Error: expect(received).toBeTruthy()
       
       Received: false
           at Proxy.<anonymous> (/Users/mathieu/Documents/Github/wp-rocket-e2e/node_modules/@playwright/test/lib/matchers/expect.js:165:37)
           at World.<anonymous> (/Users/mathieu/Documents/Github/wp-rocket-e2e/src/support/steps/lcp-beacon-script.ts:131:32)
   ✔ After # src/support/hooks.ts:146
       Attachment (image/png)
       Attachment (video/webm)


13/25 steps [=======================================================================                                                                  ] 0
24/25 steps [====================================================================================================================================     ] Expected LCP for mobile - /wp-content/rocket-test-data/images/lcp/testsvg.svg for http://mathieu.e2e.rocketlabsqa.ovh/lcp_no_dimension_svg is not present in actual - {"type":"img","src":"https://discuss-assets.s3.amazonaws.com/original/3X/1/9/19823cc1f1f887d70755e0b500dd8ce2c51ba7f9.svg"}


Expected LCP for mobile - /wp-content/rocket-test-data/images/lcp/testavif.avif for http://mathieu.e2e.rocketlabsqa.ovh/lcp_no_dimensions_picture is not present in actual - {"type":"picture","sources":[{"srcset":"http://mathieu.e2e.rocketlabsqa.ovh/wp-content/rocket-test-data/images/lcp/testwebp.webp","media":"(max-width: 700px)"},{"srcset":"http://mathieu.e2e.rocketlabsqa.ovh/wp-content/rocket-test-data/images/lcp/testjpeg.jpeg","media":"(max-width: 450px)"}],"src":""}



0
2) Scenario: Beacon captures expected images in mobile # src/features/lcp-beacon-script.feature:16
   ✔ Before # src/support/hooks.ts:132
   ✔ Given I am logged in # src/support/steps/general.ts:23
   ✔ And plugin is installed 'new_release' # src/support/steps/general.ts:29
   ✔ And plugin is activated # src/support/steps/general.ts:48
   ✔ And I go to 'wp-admin/options-general.php?page=wprocket#dashboard' # src/support/steps/general.ts:93
   ✔ And plugin 'sitepress-multilingual-cms' is deactivated # src/support/steps/general.ts:291
   ✔ Given I install plugin 'https://github.com/wp-media/wp-rocket-e2e-test-helper/raw/main/helper-plugin/force-wp-mobile.zip' # src/support/steps/general.ts:297
   ✔ And plugin 'force-wp-mobile' is activated # src/support/steps/steps.ts:51
   ✔ When I log out # src/support/steps/general.ts:136
   ✔ And I visit the urls for 'mobile' # src/support/steps/lcp-beacon-script.ts:17
   ✔ And plugin 'force-wp-mobile' is deactivated # src/support/steps/general.ts:291
   ✖ Then lcp and atf should be as expected for 'mobile' # src/support/steps/lcp-beacon-script.ts:74
       Error: expect(received).toBeTruthy()
       
       Received: false
           at Proxy.<anonymous> (/Users/mathieu/Documents/Github/wp-rocket-e2e/node_modules/@playwright/test/lib/matchers/expect.js:165:37)
           at World.<anonymous> (/Users/mathieu/Documents/Github/wp-rocket-e2e/src/support/steps/lcp-beacon-script.ts:109:32)
   ✔ After # src/support/hooks.ts:146
       Attachment (image/png)
       Attachment (video/webm)


25/25 steps [=========================================================================================================================================] 3 scenarios (2 failed, 1 passed)
25 steps (2 failed, 23 passed)
4m12.071s (executing steps: 3m35.964s)

MathieuLamiot · 2024-06-06T11:59:56Z

http://mathieu.e2e.rocketlabsqa.ovh/lcp_no_dimensions_picture/

No more Fatal. Preload tag as described by Mai has a known issue on develop already

<link rel="preload" as="image" href="http://mathieu.e2e.rocketlabsqa.ovh/wp-content/rocket-test-data/images/lcp/testavif.avif" media="(min-width: 700px)" fetchpriority="high">
<link rel="preload" as="image" href="http://mathieu.e2e.rocketlabsqa.ovh/wp-content/rocket-test-data/images/lcp/testPng.png" media="(min-width: 450px)" fetchpriority="high">

http://mathieu.e2e.rocketlabsqa.ovh/lcp_6599_template2/

lcp_bg_responsive_webkit_template/
Issue from #6634

link rel="preload" as="image" imagesrcset="https://fastly.picsum.photos/id/976/200/300.jpg?hmac=s1Uz9fgJv32r8elfaIYn7pLpQXps7X9oYNwC5XirhO8%201dppx,https://rocketlabsqa.ovh/wp-content/rocket-test-data/images/fixtheissue.jpg%202dppx" fetchpriority="high"

http://mathieu.e2e.rocketlabsqa.ovh/lcp_bg_responsive_imgset_template/
Issue from #6634
link rel="preload" as="image" imagesrcset="http://mathieu.e2e.rocketlabsqa.ovh/wp-content/rocket-test-data/images/lcp/testavif.avif%201dppx,http://mathieu.e2e.rocketlabsqa.ovh/wp-content/rocket-test-data/images/lcp/testwebp.webp%202dppx" fetchpriority="high"

https://new.rocketlabsqa.ovh/lcp_specialchar2/
All good. The preload is correct.
link rel="preload" as="image" href="http://mathieu.e2e.rocketlabsqa.ovh/wp-content/rocket-test-data/images/lcp/%D7%A2%D7%93%D7%99-%D7%A8%D7%99%D7%93%D7%9C%D7%9E%D7%9F-%D7%A8%D7%90%D7%A9%D7%99-%D7%90%D7%A8%D7%95%D7%9A-200x300.webp" fetchpriority="high"

lcp_relative_url_withspecialchar_template
All good as well. fetchprio is added to relative LCP and the preload is correct.
link rel="preload" as="image" href="http://mathieu.e2e.rocketlabsqa.ovh/wp-content/rocket-test-data/images/lcp/image%20with%20space.jpg" fetchpriority="high"

Tabrisrp · 2024-06-06T12:24:56Z

inc/Engine/Media/AboveTheFold/AJAX/Controller.php

+		// Returned objects must always have a src.
+		// It is used when applying the front-end optimization.
+		if ( ! isset( $object->src ) ) {
+			$object->src = '';
+		}


This is not entirely true, bg-img and bg-img-set don't use src on the main object.

While that doesn't break anything, I think it's good to have it noted somewhere.

You are absolutely right @Tabrisrp. Thanks for for pointing it out. I adapted the comment. WDYT?

yes looks good to me

Doesn't look good to the linter :'( On it

Remove duplicated code

c2ec494

Fallback value for src attribute if not set

MathieuLamiot linked an issue Jun 5, 2024 that may be closed by this pull request

picture and img-srcset have no src attribute in the database, making the front end optimization crash #6699

Closed

Fix linter

67ab4ef

MathieuLamiot added 3 commits June 5, 2024 22:13

Fix tests: when bg_set exists, src must be an empty string

7d65045

Fix order of json attributes in expected results

781bd6b

Fix json message for tests

64ea5d2

MathieuLamiot requested a review from a team June 6, 2024 12:24

Tabrisrp approved these changes Jun 6, 2024

View reviewed changes

MathieuLamiot added 2 commits June 6, 2024 14:31

Adjusted comment based on feedback

437d418

Linter fix :D - Never let a manager code, they break stuff all the time!

7cc4788

MathieuLamiot added this pull request to the merge queue Jun 6, 2024

Merged via the queue into develop with commit 78395ea Jun 6, 2024
13 checks passed

MathieuLamiot deleted the fix/6699-set-src-to-all-oci-objects branch June 6, 2024 12:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fallback value for src attribute if not set #6700

Fallback value for src attribute if not set #6700

MathieuLamiot commented Jun 5, 2024

MathieuLamiot commented Jun 5, 2024 •

edited

Loading

Tabrisrp commented Jun 5, 2024

codacy-production bot commented Jun 5, 2024 •

edited

Loading

Mai-Saad commented Jun 6, 2024 •

edited

Loading

MathieuLamiot commented Jun 6, 2024

MathieuLamiot commented Jun 6, 2024 •

edited

Loading

Tabrisrp Jun 6, 2024

MathieuLamiot Jun 6, 2024

Tabrisrp Jun 6, 2024

MathieuLamiot Jun 6, 2024

Fallback value for src attribute if not set #6700

Fallback value for src attribute if not set #6700

Conversation

MathieuLamiot commented Jun 5, 2024

Description

Documentation

User documentation

Technical documentation

Type of change

New dependencies

Risks

Checklists

Feature validation

Documentation

Code style

Observability

Risks

MathieuLamiot commented Jun 5, 2024 • edited Loading

Tabrisrp commented Jun 5, 2024

codacy-production bot commented Jun 5, 2024 • edited Loading

Coverage summary from Codacy

See diff coverage on Codacy

See your quality gate settings Change summary preferences

Footnotes

Mai-Saad commented Jun 6, 2024 • edited Loading

MathieuLamiot commented Jun 6, 2024

MathieuLamiot commented Jun 6, 2024 • edited Loading

Tabrisrp Jun 6, 2024

Choose a reason for hiding this comment

MathieuLamiot Jun 6, 2024

Choose a reason for hiding this comment

Tabrisrp Jun 6, 2024

Choose a reason for hiding this comment

MathieuLamiot Jun 6, 2024

Choose a reason for hiding this comment

MathieuLamiot commented Jun 5, 2024 •

edited

Loading

codacy-production bot commented Jun 5, 2024 •

edited

Loading

Mai-Saad commented Jun 6, 2024 •

edited

Loading

MathieuLamiot commented Jun 6, 2024 •

edited

Loading