Skip to content

Change minimum viewport width to be exclusive whereas the maximum width remains inclusive #1839

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
westonruter merged 5 commits into from
Feb 3, 2025
Merged

Change minimum viewport width to be exclusive whereas the maximum width remains inclusive #1839

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9e59c8b
Change minimum viewport width to be exclusive whereas the maximum wid…
westonruter Feb 1, 2025
b503cf3
Improve specificity of types
westonruter Feb 1, 2025
5915e81
Avoid dumping PHP_INT_MAX to frontend
westonruter Feb 1, 2025
0dafbb7
Remove needless array_map with JsonSerializable
westonruter Feb 2, 2025
b4ea6bd
Eliminate use of PHP_INT_MAX to represent the maximum viewport width
westonruter Feb 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 19 additions & 14 deletions plugins/optimization-detective/class-od-html-tag-processor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ final class OD_HTML_Tag_Processor extends WP_HTML_Tag_Processor {
* Open stack tags.
*
* @since 0.4.0
* @var string[]
* @var non-empty-string[]
*/
private $open_stack_tags = array();

Expand All @@ -160,15 +160,15 @@ final class OD_HTML_Tag_Processor extends WP_HTML_Tag_Processor {
* are children of the `BODY` tag. This is used in {@see self::get_xpath()}.
*
* @since 1.0.0
* @var array<array<string, string>>
* @var array<array<non-empty-string, string>>
*/
private $open_stack_attributes = array();

/**
* Open stack indices.
*
* @since 0.4.0
* @var int[]
* @var non-negative-int[]
*/
private $open_stack_indices = array();

Expand All @@ -181,7 +181,7 @@ final class OD_HTML_Tag_Processor extends WP_HTML_Tag_Processor {
* populated back into `$this->open_stack_tags` and `$this->open_stack_indices`.
*
* @since 0.4.0
* @var array<string, array{tags: string[], attributes: array<array<string, string>>, indices: int[]}>
* @var array<string, array{tags: non-empty-string[], attributes: array<array<non-empty-string, string>>, indices: non-negative-int[]}>
*/
private $bookmarked_open_stacks = array();

Expand Down Expand Up @@ -221,7 +221,7 @@ final class OD_HTML_Tag_Processor extends WP_HTML_Tag_Processor {
* Mapping of bookmark name to a list of HTML strings which will be inserted at the time get_updated_html() is called.
*
* @since 0.4.0
* @var array<string, string[]>
* @var array<non-empty-string, string[]>
*/
private $buffered_text_replacements = array();

Expand All @@ -238,7 +238,7 @@ final class OD_HTML_Tag_Processor extends WP_HTML_Tag_Processor {
* Count for the number of times that the cursor was moved.
*
* @since 0.6.0
* @var int
* @var non-negative-int
* @see self::next_token()
* @see self::seek()
*/
Expand Down Expand Up @@ -292,7 +292,7 @@ public function next_open_tag(): bool {
* @see WP_HTML_Processor::expects_closer()
* @since 0.4.0
*
* @param string|null $tag_name Tag name, if not provided then the current tag is used. Optional.
* @param non-empty-string|null $tag_name Tag name, if not provided then the current tag is used. Optional.
* @return bool Whether to expect a closer for the tag.
*/
public function expects_closer( ?string $tag_name = null ): bool {
Expand Down Expand Up @@ -336,6 +336,11 @@ public function next_token(): bool {
if ( null === $tag_name || $this->get_token_type() !== '#tag' ) {
return true;
}
/**
* Tag name.
*
* @var non-empty-string $tag_name
*/

if ( $this->previous_tag_without_closer ) {
array_pop( $this->open_stack_tags );
Expand Down Expand Up @@ -422,7 +427,7 @@ public function next_token(): bool {
* @see self::next_token()
* @see self::seek()
*
* @return int Count of times the cursor has moved.
* @return non-negative-int Count of times the cursor has moved.
*/
public function get_cursor_move_count(): int {
return $this->cursor_move_count;
Expand Down Expand Up @@ -458,8 +463,8 @@ public function set_attribute( $name, $value ): bool { // phpcs:ignore SlevomatC
*
* @since 0.4.0
*
* @param string $name Meta attribute name.
* @param string|true $value Value.
* @param non-empty-string $name Meta attribute name.
* @param string|true $value Value.
* @return bool Whether an attribute was set.
*/
public function set_meta_attribute( string $name, $value ): bool {
Expand Down Expand Up @@ -489,7 +494,7 @@ public function remove_attribute( $name ): bool { // phpcs:ignore SlevomatCoding
* @since 0.4.0
* @see WP_HTML_Processor::get_current_depth()
*
* @return int Nesting-depth of current location in the document.
* @return non-negative-int Nesting-depth of current location in the document.
*/
public function get_current_depth(): int {
return count( $this->open_stack_tags );
Expand Down Expand Up @@ -565,7 +570,7 @@ public function release_bookmark( $name ): bool {
* @since 0.4.0
* @since 0.9.0 Renamed from get_breadcrumbs() to get_indexed_breadcrumbs().
*
* @return Generator<array{string, int, array<string, string>}> Breadcrumb.
* @return Generator<array{non-empty-string, non-negative-int, array<non-empty-string, string>}> Breadcrumb.
*/
private function get_indexed_breadcrumbs(): Generator {
foreach ( $this->open_stack_tags as $i => $breadcrumb_tag_name ) {
Expand All @@ -584,7 +589,7 @@ private function get_indexed_breadcrumbs(): Generator {
*
* @since 1.0.0
*
* @return array<string, string> Disambiguating attributes.
* @return array<non-empty-string, string> Disambiguating attributes.
*/
private function get_disambiguating_attributes(): array {
$attributes = array();
Expand Down Expand Up @@ -617,7 +622,7 @@ private function get_disambiguating_attributes(): array {
* @since 0.9.0
* @see WP_HTML_Processor::get_breadcrumbs()
*
* @return string[] Array of tag names representing path to matched node.
* @return non-empty-string[] Array of tag names representing path to matched node.
*/
public function get_breadcrumbs(): array {
return $this->open_stack_tags;
Expand Down
14 changes: 7 additions & 7 deletions plugins/optimization-detective/class-od-link-collection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
* @phpstan-type Link array{
* attributes: LinkAttributes,
* minimum_viewport_width: int<0, max>|null,
* maximum_viewport_width: positive-int|null
* maximum_viewport_width: int<1, max>|null
* }
*
* @phpstan-type LinkAttributes array{
Expand Down Expand Up @@ -53,9 +53,9 @@ final class OD_Link_Collection implements Countable {
*
* @phpstan-param LinkAttributes $attributes
*
* @param array $attributes Attributes.
* @param int<0, max>|null $minimum_viewport_width Minimum width or null if not bounded or relevant.
* @param positive-int|null $maximum_viewport_width Maximum width or null if not bounded (i.e. infinity) or relevant.
* @param array $attributes Attributes.
* @param int<0, max>|null $minimum_viewport_width Minimum width (exclusive) or null if not bounded or relevant.
* @param int<1, max>|null $maximum_viewport_width Maximum width (inclusive) or null if not bounded (i.e. infinity) or relevant.
*
* @throws InvalidArgumentException When invalid arguments are provided.
*/
Expand Down Expand Up @@ -194,9 +194,9 @@ static function ( array $carry, array $link ): array {
&&
is_int( $last_link['maximum_viewport_width'] )
&&
$last_link['maximum_viewport_width'] + 1 === $link['minimum_viewport_width']
$last_link['maximum_viewport_width'] === $link['minimum_viewport_width']
Copy link
Copy Markdown
Member Author

@westonruter westonruter Feb 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Key change. The addition of 1 is no longer needed because the previous maximum is intended to equal the next minimum.

) {
$last_link['maximum_viewport_width'] = max( $last_link['maximum_viewport_width'], $link['maximum_viewport_width'] );
$last_link['maximum_viewport_width'] = null === $link['maximum_viewport_width'] ? null : max( $last_link['maximum_viewport_width'], $link['maximum_viewport_width'] );

// Update the last link with the new maximum viewport width.
$carry[ count( $carry ) - 1 ] = $last_link;
Expand Down Expand Up @@ -249,7 +249,7 @@ public function get_html(): string {
/**
* Constructs the Link HTTP response header.
*
* @return string|null Link HTTP response header, or null if there are none.
* @return non-empty-string|null Link HTTP response header, or null if there are none.
*/
public function get_response_header(): ?string {
$link_headers = array();
Expand Down
12 changes: 6 additions & 6 deletions plugins/optimization-detective/class-od-tag-visitor-registry.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ final class OD_Tag_Visitor_Registry implements Countable, IteratorAggregate {
/**
* Visitors.
*
* @var array<string, TagVisitorCallback>
* @var array<non-empty-string, TagVisitorCallback>
*/
private $visitors = array();

Expand All @@ -36,8 +36,8 @@ final class OD_Tag_Visitor_Registry implements Countable, IteratorAggregate {
*
* @phpstan-param TagVisitorCallback $tag_visitor_callback
*
* @param string $id Identifier for the tag visitor.
* @param callable $tag_visitor_callback Tag visitor callback.
* @param non-empty-string $id Identifier for the tag visitor.
* @param callable $tag_visitor_callback Tag visitor callback.
*/
public function register( string $id, callable $tag_visitor_callback ): void {
$this->visitors[ $id ] = $tag_visitor_callback;
Expand All @@ -46,7 +46,7 @@ public function register( string $id, callable $tag_visitor_callback ): void {
/**
* Determines if a visitor has been registered.
*
* @param string $id Identifier for the tag visitor.
* @param non-empty-string $id Identifier for the tag visitor.
* @return bool Whether registered.
*/
public function is_registered( string $id ): bool {
Expand All @@ -56,7 +56,7 @@ public function is_registered( string $id ): bool {
/**
* Gets a registered visitor.
*
* @param string $id Identifier for the tag visitor.
* @param non-empty-string $id Identifier for the tag visitor.
* @return TagVisitorCallback|null Whether registered.
*/
public function get_registered( string $id ): ?callable {
Expand All @@ -69,7 +69,7 @@ public function get_registered( string $id ): ?callable {
/**
* Unregisters a tag visitor.
*
* @param string $id Identifier for the tag visitor.
* @param non-empty-string $id Identifier for the tag visitor.
* @return bool Whether a tag visitor was unregistered.
*/
public function unregister( string $id ): bool {
Expand Down
54 changes: 26 additions & 28 deletions plugins/optimization-detective/class-od-url-metric-group-collection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,26 +47,21 @@ final class OD_URL_Metric_Group_Collection implements Countable, IteratorAggrega
/**
* Breakpoints in max widths.
*
* Valid values are from 1 to PHP_INT_MAX - 1. This is because:
*
* 1. It doesn't make sense for there to be a viewport width of zero, so the first breakpoint (max width) must be at least 1.
* 2. After the last breakpoint, the final breakpoint group is set to be spanning one plus the last breakpoint max width up
* until PHP_INT_MAX. So a breakpoint cannot be PHP_INT_MAX because then the minimum viewport width for the final group
* would end up being larger than PHP_INT_MAX.
* A breakpoint must be greater than zero because a viewport group's maximum viewport width has a minimum (inclusive)
* value of 1, and the breakpoints are used as the maximum viewport widths for the viewport groups, with the addition of
* a final viewport group which has a maximum viewport width of infinity.
*
* This array may be empty in which case there are no responsive breakpoints and all URL Metrics are collected in a
* single group.
*
* @var int[]
* @phpstan-var positive-int[]
* @var positive-int[]
*/
private $breakpoints;

/**
* Sample size for URL Metrics for a given breakpoint.
*
* @var int
* @phpstan-var positive-int
* @var int<1, max>
*/
private $sample_size;

Expand All @@ -75,8 +70,7 @@ final class OD_URL_Metric_Group_Collection implements Countable, IteratorAggrega
*
* A freshness age of zero means a URL Metric will always be considered stale.
*
* @var int
* @phpstan-var 0|positive-int
* @var int<0, max>
*/
private $freshness_ttl;

Expand All @@ -91,7 +85,7 @@ final class OD_URL_Metric_Group_Collection implements Countable, IteratorAggrega
* get_groups_by_lcp_element?: array<string, OD_URL_Metric_Group[]>,
* get_common_lcp_element?: OD_Element|null,
* get_all_element_max_intersection_ratios?: array<string, float>,
* get_xpath_elements_map?: array<string, non-empty-array<int, OD_Element>>,
* get_xpath_elements_map?: array<string, non-empty-array<non-negative-int, OD_Element>>,
* get_all_elements_positioned_in_any_initial_viewport?: array<string, bool>,
* }
*/
Expand All @@ -102,6 +96,10 @@ final class OD_URL_Metric_Group_Collection implements Countable, IteratorAggrega
*
* @throws InvalidArgumentException When an invalid argument is supplied.
*
* @phpstan-param positive-int[] $breakpoints
* @phpstan-param int<1, max> $sample_size
* @phpstan-param int<0, max> $freshness_ttl
*
* @param OD_URL_Metric[] $url_metrics URL Metrics.
* @param non-empty-string $current_etag The current ETag.
* @param int[] $breakpoints Breakpoints in max widths.
Expand All @@ -127,13 +125,13 @@ public function __construct( array $url_metrics, string $current_etag, array $br
sort( $breakpoints );
$breakpoints = array_values( array_unique( $breakpoints, SORT_NUMERIC ) );
foreach ( $breakpoints as $breakpoint ) {
if ( ! is_int( $breakpoint ) || $breakpoint < 1 || PHP_INT_MAX === $breakpoint ) {
if ( ! is_int( $breakpoint ) || $breakpoint < 1 ) {
throw new InvalidArgumentException(
esc_html(
sprintf(
/* translators: %d is the invalid breakpoint */
__(
'Each of the breakpoints must be greater than zero and less than PHP_INT_MAX, but encountered: %d',
'Each of the breakpoints must be greater than zero, but encountered: %d',
'optimization-detective'
),
$breakpoint
Expand Down Expand Up @@ -215,7 +213,7 @@ public function get_first_group(): OD_URL_Metric_Group {
*
* This group normally represents viewports for desktop devices. This group always has a minimum viewport width
* defined as one greater than the largest breakpoint returned by {@see od_get_breakpoint_max_widths()}.
* The maximum viewport is always `PHP_INT_MAX`, or in other words it is unbounded.
* The maximum viewport width of this group is always `null`, or in other words it is unbounded.
*
* @since 0.7.0
*
Expand Down Expand Up @@ -244,13 +242,13 @@ public function clear_cache(): void {
* @return OD_URL_Metric_Group[] Groups.
*/
private function create_groups(): array {
$groups = array();
$min_width = 0;
foreach ( $this->breakpoints as $max_width ) {
$groups[] = new OD_URL_Metric_Group( array(), $min_width, $max_width, $this->sample_size, $this->freshness_ttl, $this );
$min_width = $max_width + 1;
$groups = array();
$min_width_exclusive = 0;
foreach ( $this->breakpoints as $max_width_inclusive ) {
$groups[] = new OD_URL_Metric_Group( array(), $min_width_exclusive, $max_width_inclusive, $this->sample_size, $this->freshness_ttl, $this );
$min_width_exclusive = $max_width_inclusive;
}
$groups[] = new OD_URL_Metric_Group( array(), $min_width, PHP_INT_MAX, $this->sample_size, $this->freshness_ttl, $this );
$groups[] = new OD_URL_Metric_Group( array(), $min_width_exclusive, null, $this->sample_size, $this->freshness_ttl, $this );
return $groups;
}

Expand All @@ -272,7 +270,7 @@ public function add_url_metric( OD_URL_Metric $new_url_metric ): void {
}
}
// @codeCoverageIgnoreStart
// In practice this exception should never get thrown because create_groups() creates groups from a minimum of 0 to a maximum of PHP_INT_MAX.
// In practice this exception should never get thrown because create_groups() creates groups from a minimum of 0 to an unbounded maximum.
throw new InvalidArgumentException(
esc_html__( 'No group available to add URL Metric to.', 'optimization-detective' )
);
Expand All @@ -285,7 +283,7 @@ public function add_url_metric( OD_URL_Metric $new_url_metric ): void {
* @since 0.1.0
* @throws InvalidArgumentException When there is no group for the provided viewport width. This would only happen if a negative width is provided.
*
* @param int $viewport_width Viewport width.
* @param positive-int $viewport_width Viewport width.
* @return OD_URL_Metric_Group URL Metric group for the viewport width.
*/
public function get_group_for_viewport_width( int $viewport_width ): OD_URL_Metric_Group {
Expand All @@ -300,7 +298,7 @@ public function get_group_for_viewport_width( int $viewport_width ): OD_URL_Metr
}
}
// @codeCoverageIgnoreStart
// In practice this exception should never get thrown because create_groups() creates groups from a minimum of 0 to a maximum of PHP_INT_MAX.
// In practice this exception should never get thrown because create_groups() creates groups from a minimum of 0 to an unbounded maximum.
throw new InvalidArgumentException(
esc_html(
sprintf(
Expand Down Expand Up @@ -495,7 +493,7 @@ public function get_common_lcp_element(): ?OD_Element {
*
* @since 0.7.0
*
* @return array<string, non-empty-array<int, OD_Element>> Keys are XPaths and values are the element instances.
* @return array<string, non-empty-array<non-negative-int, OD_Element>> Keys are XPaths and values are the element instances.
*/
public function get_xpath_elements_map(): array {
if ( array_key_exists( __FUNCTION__, $this->result_cache ) ) {
Expand Down Expand Up @@ -667,8 +665,8 @@ public function count(): int {
* every_group_populated: bool,
* groups: array<int, array{
* lcp_element: ?OD_Element,
* minimum_viewport_width: 0|positive-int,
* maximum_viewport_width: positive-int,
* minimum_viewport_width: int<0, max>,
* maximum_viewport_width: int<1, max>|null,
* complete: bool,
* url_metrics: OD_URL_Metric[]
* }>
Expand Down
Loading