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

Fix static-analysis and PHPUnit tests errors #7814

Merged
merged 4 commits into from
Jun 7, 2024
Merged

Conversation

thelovekesh
Copy link
Collaborator

@thelovekesh thelovekesh commented Jun 5, 2024

Summary

PHPStan

The latest PHPStan version (1.11.3) is showing a few errors:

[errors]
 ------ ---------------------------------------------------------------------- 
  Line   includes/sanitizers/class-amp-comments-sanitizer.php                  
 ------ ---------------------------------------------------------------------- 
  45     Call to AMP_Base_Sanitizer::init() on a separate line has no effect.  
 ------ ---------------------------------------------------------------------- 

 ------ ---------------------------------------------------------------------- 
  Line   includes/sanitizers/class-amp-script-sanitizer.php                    
 ------ ---------------------------------------------------------------------- 
  102    Call to AMP_Base_Sanitizer::init() on a separate line has no effect.  
 ------ ---------------------------------------------------------------------- 

 ------ ---------------------------------------------------------------------- 
  Line   includes/sanitizers/class-amp-style-sanitizer.php                     
 ------ ---------------------------------------------------------------------- 
  894    Call to AMP_Base_Sanitizer::init() on a separate line has no effect.  
 ------ ---------------------------------------------------------------------- 

This PR includes the fix to resolve these static analysis errors.

PHPUnit

A few tests for ReaderThemeSupportFeatures failed due to new changes introduced in theme.json v3. The new versions introduce a new settings.spacing.defaultSpacingSizes property which decides if default space sizing needs to be preferred over the default ones. This PR adds support for this property in reader themes as well. Also see the dev notes for this new change - WordPress/gutenberg#58409 (comment)

Checklist

  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

Copy link
Contributor

github-actions bot commented Jun 5, 2024

Plugin builds for 5986229 are ready 🛎️!

Checksums
# Development build checksums
0ea508aa5064fb79e58f531de24b812c1e55b209c6e0bd1119e4c7c8dc8cfd63 *amp.zip

# Production build checksums
37fa88f69fe8140aff8e5cf5727d81af32857a4ece9d92ebe757a9ab9c9b9acf *amp.zip

Warning

These builds are for testing purposes only and should not be used in production.

@thelovekesh thelovekesh changed the title Add PHPStan false-positives in baseline file Fix static-analysis errors Jun 5, 2024
@thelovekesh thelovekesh removed the request for review from westonruter June 5, 2024 21:50
@thelovekesh thelovekesh marked this pull request as draft June 5, 2024 21:50
@thelovekesh
Copy link
Collaborator Author

Marking as draft now, as I'll be fixing the failing tests in trunk in same PR.

@thelovekesh thelovekesh changed the title Fix static-analysis errors Fix static-analysis and PHPUnit tests errors Jun 6, 2024
@thelovekesh thelovekesh marked this pull request as ready for review June 6, 2024 10:24
Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

Looks good, just with some minor nits.

Pre-approving.

src/ReaderThemeSupportFeatures.php Outdated Show resolved Hide resolved
src/ReaderThemeSupportFeatures.php Outdated Show resolved Hide resolved
src/ReaderThemeSupportFeatures.php Outdated Show resolved Hide resolved
thelovekesh and others added 2 commits June 7, 2024 01:58
@thelovekesh thelovekesh merged commit 22012c5 into develop Jun 7, 2024
32 of 34 checks passed
@thelovekesh thelovekesh deleted the fix/phpstan-errors branch June 7, 2024 07:46
@thelovekesh thelovekesh added this to the v2.5.4 milestone Jul 9, 2024
@thelovekesh
Copy link
Collaborator Author

QA Passed ✅

  • Static analysis and PHPUnit tests are all 🟢 on the develop branch.
  • Regarding the change in ReaderThemeSupportFeatures's theme.json v3:
    • Spacing sizes styles are being included in reader mode.
    • If theme.json version is set to 3, and defaultSpacingSizes is set to true, the default spacing sizes styles are being added as expected in reader mode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants