Skip to content
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.

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

[TASK] Add (and use) a CSSListItem type #1212

Merged
merged 2 commits into from
Mar 25, 2025
Merged

Conversation

JakeQZ
Copy link
Contributor

@JakeQZ JakeQZ commented Mar 24, 2025

This allows a single type to be used for the contents of a CSSList, instead of a long list of orred types, and helps with static analysis.

Various assertInstanceOf() tests are added to the test cases to confirm that the list items are of the type expected.

Some implements and exetends lists are now alphabetically sorted.

@JakeQZ JakeQZ added cleanup blocked Needs another issue to be resolved first labels Mar 24, 2025
@JakeQZ JakeQZ requested a review from oliverklee March 24, 2025 02:23
@JakeQZ JakeQZ self-assigned this Mar 24, 2025
@JakeQZ
Copy link
Contributor Author

JakeQZ commented Mar 24, 2025

Notes:

  • I have marked this as blocked by [CLEANUP] Return null from DeclarationBlock::parse() on failure #1209, which is a pre-patch for this;
    • This PR includes that change, so if that PR is accepted, it should be possible to merge this PR without conflict;
  • When a type is A|B, PHPStan is happy if either A or B has the method invoked,
    • but if A and B both implement C, and the type is C, it requires that C defines the method, or an additional type check is performed - hence the need for additional assertInstanceOf() checks in the tests;
  • I think the new PHPStan error arises because it would now be possible to have CSSListItem|array as a native parameter type - but not in PHP 7.2 -
    • It would be nice if PHPStan would allow specification of a target PHP version separately from the version it runs on, so it would not generate errors that can't be fixed for the minimum target PHP version;
      • IIRC, this was not possible with Psalm, but maybe it is possible with PHPStan;
      • Running PHPStan on PHP 7.2 would almost certainly not permit the latest version.

@coveralls
Copy link

coveralls commented Mar 24, 2025

Coverage Status

coverage: 52.048%. remained the same
when pulling 691cfb5 on task/csslistitem-type
into acfe85e on main.

@JakeQZ JakeQZ marked this pull request as draft March 24, 2025 02:31
@JakeQZ
Copy link
Contributor Author

JakeQZ commented Mar 24, 2025

Apparently PHP<7.4 does not like implementing an interface that is already implemented. That was the other note I was going to add - that I kept that redundancy in for clarity and completeness. Seems we can't :/

JakeQZ added 2 commits March 25, 2025 01:29
This allows a single type to be used for the contents of a `CSSList`,
instead of a long list of orred types, and helps with static analysis.

Various `assertInstanceOf()` tests are added to the test cases to confirm that
the list items are of the type expected.

Some `implements` and `exetends` lists are now alphabetically sorted.
PHP<7.4 does not allow this.
Instead, for clarity,
add a DocBlock comment stating which additional interfaces should be
implemented that are not explicitly listed in the `implements` section.

When our minimum PHP version becomes 7.4 or above, we can revisit this.
@JakeQZ JakeQZ force-pushed the task/csslistitem-type branch from c9db265 to 691cfb5 Compare March 25, 2025 01:30
@JakeQZ JakeQZ marked this pull request as ready for review March 25, 2025 01:30
@JakeQZ
Copy link
Contributor Author

JakeQZ commented Mar 25, 2025

I've fixed the issues with PHP<7.4, and rebased. This can be reviewed now.

@oliverklee oliverklee merged commit 734c72e into main Mar 25, 2025
21 checks passed
@oliverklee oliverklee deleted the task/csslistitem-type branch March 25, 2025 09:12
JakeQZ added a commit that referenced this pull request Mar 30, 2025
JakeQZ added a commit that referenced this pull request Mar 30, 2025
oliverklee pushed a commit that referenced this pull request Mar 31, 2025
JakeQZ added a commit that referenced this pull request Apr 8, 2025
These should probably have been added as part of #1212.
They confirm that the various types supplanted by `CSSListItem` in the API all
implement the new interface.

Resolves #1236.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Needs another issue to be resolved first cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants