Introduce OD_Element class and improve PHP API by westonruter · Pull Request #1585 · WordPress/performance

westonruter · 2024-10-09T21:00:42Z

See https://github.com/WordPress/performance/pull/1373/files#r1792652840:

See note below on the get_all_denormalized_elements() method that I'd like to instead have $element be an OD_Element class instance which has a $url_metric property to access the OD_URL_Metric instance if is a part of. In the same way, the OD_URL_Metric class can have a $group property to indicate the OD_URL_Metric_Group it is a part of. This would eliminate the need to pass back an array of tuples and instead. So instead, to get the $group this code could do $element->url_metric->group.

The OD_Element class implements ArrayAccess so it is fully backwards-compatible with using values as arrays. So where existing plugins may do $element['isLCP'] this will still continue to work, although $element->is_lcp() would now be preferable. There is also a $element->get() method which allows accessing extended element data. This is closely related to #1492 which did the same for OD_URL_Metric.

plugins/optimization-detective/class-od-url-metric.php

westonruter · 2024-10-09T21:04:35Z

plugins/optimization-detective/class-od-url-metric-group-collection.php

+	 * @return array<string, non-empty-array<int, OD_Element>> Keys are XPaths and values are the element instances.
 	 */
-	public function get_all_denormalized_elements(): array {
+	public function get_all_elements(): array {


I don't really like the get_all_elements name. It's more like get_elements_grouped_by_xpath.

How about get_all_elements_by_xpath()?

That works, but then it would seem like you could pass an $xpath to get just the elements with the given XPath.

Like document.getElementById()

That's fair, though I'm not sure how including the word grouped would clarify that. FWIW to me that all helps to clarify this will return "all" elements, not just the elements filtered by something.

How about get_xpath_elements_map()?

Done in 98d3fd8

plugins/optimization-detective/class-od-element.php

…ents

github-actions · 2024-10-16T21:09:44Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: westonruter <[email protected]>
Co-authored-by: felixarntz <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

westonruter · 2024-10-16T21:10:01Z

@felixarntz Thoughts on this?

felixarntz

@westonruter This looks great so far. Only a few points of feedback.

felixarntz · 2024-10-17T16:16:28Z

plugins/embed-optimizer/class-embed-optimizer-tag-visitor.php

+			$resized_bounding_client_rect = $element->get( 'resizedBoundingClientRect' );
+			if ( ! is_array( $resized_bounding_client_rect ) ) {
 				continue;
 			}
+			$group           = $element->url_metric->group;
 			$group_min_width = $group->get_minimum_viewport_width();
 			if ( ! isset( $minimums[ $group_min_width ] ) ) {
 				$minimums[ $group_min_width ] = array(
 					'group'  => $group,
-					'height' => $element['resizedBoundingClientRect']['height'],
+					'height' => $resized_bounding_client_rect['height'],
 				);
 			} else {
 				$minimums[ $group_min_width ]['height'] = min(
 					$minimums[ $group_min_width ]['height'],
-					$element['resizedBoundingClientRect']['height']
+					$resized_bounding_client_rect['height']


Just curious: Wouldn't all this marked code also have worked in the way it was before this change, given OD_Element implements ArrayAccess?

Yes, it would have worked the same. I made this change because of a PHPStan issue where attempting to access $element['resizedBoundingClientRect']['height'] results in a PHPStan error:

phpstan: Cannot access offset 'height' on array{width: float, height: float, x: float, y: float, top: float, right: float, bottom: float, left: float}|bool|float|non-empty-string.

PHPStan isn't constraining the type based on the type of $element['resizedBoundingClientRect'] based on the key resizedBoundingClientRect so the possible types are a union of all values in ElementData, which also doesn't necessarily include this extended resizedBoundingClientRect key's type (although here it is a DOMRect).

This goes back to wanting to be able to provide richer typing in PHPStan for getters like this, but in the meantime resorting to using OD_Element::get() which returns mixed and then checking the return value as being an array avoids the PHPStan error.

Although I'm a bit surprised it's not complaining that the $resized_bounding_client_rect['height'] key may not be defined since if I do PHPStan\dumpType($resized_bounding_client_rect) it is typed as just array. But this appears to be as expected: https://phpstan.org/r/bc084e48-691a-4232-be15-7b806fda8b3e. I recall there was some PHPStan extension to make checks even more strict than strict rules mandate all array keys be checked as well, but I can't find how to do that off hand.

plugins/optimization-detective/class-od-element.php

felixarntz · 2024-10-17T16:19:32Z

plugins/optimization-detective/class-od-url-metric-group-collection.php

+	 * @return array<string, non-empty-array<int, OD_Element>> Keys are XPaths and values are the element instances.
 	 */
-	public function get_all_denormalized_elements(): array {
+	public function get_all_elements(): array {


How about get_all_elements_by_xpath()?

felixarntz · 2024-10-17T16:23:53Z

plugins/optimization-detective/class-od-element.php

+	 * @var OD_URL_Metric
+	 * @readonly
+	 */
+	public $url_metric;


Let's not use a public property, as we wouldn't want this to be writable (the annotation is not enough). Can you instead add e.g. a get_url_metric() method? For convenience, you may even want to add a get_url_metric_group() or get_group() method as a shorthand, given that $group was previously easily accessible in the return value of the get_all_denormalized_elements() method.

What about the public $group property on OD_URL_Metric? It has to be mutable since \OD_URL_Metric_Group::add_url_metric() needs to be able to assign it to the group when it is added.

I must have missed that one before. Better to use a setter then for OD_URL_Metric::$group.

For this one though, would we even need to set it?

No, I don't think there is a need for a set_url_metric(). I'll add a getter for both and a setter for one.

Done in 344d6e1

Co-authored-by: Felix Arntz <[email protected]>

… add/od-element-class

felixarntz

@westonruter Thanks, LGTM!

… add/od-element-class

westonruter added the [Plugin] Optimization Detective Issues for the Optimization Detective plugin label Oct 9, 2024

westonruter added this to the optimization-detective n.e.x.t milestone Oct 9, 2024

westonruter mentioned this pull request Oct 9, 2024

Leverage URL metrics to reserve space for embeds to reduce CLS #1373

Merged

2 tasks

westonruter commented Oct 9, 2024

View reviewed changes

plugins/optimization-detective/class-od-url-metric.php Show resolved Hide resolved

westonruter commented Oct 9, 2024

View reviewed changes

plugins/optimization-detective/class-od-element.php Outdated Show resolved Hide resolved

westonruter added the [Type] Enhancement A suggestion for improvement of an existing feature label Oct 9, 2024

Base automatically changed from add/embed-optimizer-min-height-reservation to trunk October 14, 2024 17:38

westonruter mentioned this pull request Oct 16, 2024

Choose smaller poster image size based on actual dimensions #1595

Merged

westonruter added 2 commits October 16, 2024 13:04

Add OD_Element class

7c96ba0

Change get_all_denormalized_elements into get_all_elements

c3e55f9

westonruter force-pushed the add/od-element-class branch from f8485bd to c3e55f9 Compare October 16, 2024 20:04

Implement getter methods

57964fb

westonruter force-pushed the add/od-element-class branch from 3de4632 to 57964fb Compare October 16, 2024 20:54

Ensure get('elements') and get_elements() return the same cached elem…

5dfcb07

…ents

westonruter marked this pull request as ready for review October 16, 2024 21:09

westonruter requested review from adamsilverstein and felixarntz as code owners October 16, 2024 21:09

westonruter added 4 commits October 16, 2024 15:27

Fix remaining XPath indices to be 1-based for #1191

9213455

Add tests for OD_Element

b5c9703

Add todos for future improved typing

f2c2d31

Reuse helper methods

9c7b154

felixarntz reviewed Oct 17, 2024

View reviewed changes

westonruter and others added 5 commits October 17, 2024 11:11

Improve exception messages

20f4ccb

Co-authored-by: Felix Arntz <[email protected]>

Use instance check rather than null check

c5c3884

Add missing since tags

8992957

Add typing for resizedBoundingClientRect

a99208b

Add getter/setter methods

344d6e1

Remove readonly tag since member is now protected

7cfdcac

westonruter requested a review from felixarntz October 17, 2024 19:41

westonruter added 5 commits October 17, 2024 12:49

Rename get_all_elements to get_xpath_elements_map

98d3fd8

Reuse is_viewport_width_in_range method

904c641

Merge branch 'trunk' of https://github.com/WordPress/performance into…

feed71b

… add/od-element-class

Work around PHPStan limitation

13c7d53

Merge branch 'trunk' into add/od-element-class

26637ed

felixarntz approved these changes Oct 18, 2024

View reviewed changes

westonruter added 2 commits October 18, 2024 08:20

Merge branch 'trunk' of https://github.com/WordPress/performance into…

61f53df

… add/od-element-class

Use get_xpath instead of offset

a070c01

westonruter merged commit 810673d into trunk Oct 18, 2024

westonruter deleted the add/od-element-class branch October 18, 2024 15:43

westonruter mentioned this pull request Oct 20, 2024

Add ReturnTypeWillChange attribute for offsetGet to suppress notice #1607

Merged

westonruter changed the title ~~Introduce OD_Element class~~ Introduce OD_Element class and improve PHP API Oct 21, 2024

Conversation

westonruter commented Oct 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Oct 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

westonruter commented Oct 16, 2024

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

westonruter Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

westonruter commented Oct 9, 2024 •

edited

Loading

github-actions bot commented Oct 16, 2024 •

edited

Loading

westonruter Oct 17, 2024 •

edited

Loading