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

Update composer packages; Bump PHP to 7.4 and WP to 5.3 #7613

Merged
merged 42 commits into from Sep 11, 2023

Conversation

thelovekesh
Copy link
Collaborator

@thelovekesh thelovekesh commented Sep 3, 2023

Summary

  • Update composer dev packages to the latest version.
  • Bump PHP to 7.4
  • Bump minimum WP version to 5.3
  • Update workflows to use globally installed PHPUnit.

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).

composer.json Outdated Show resolved Hide resolved
@thelovekesh thelovekesh changed the title Update composer packages; Fix PHPCS errors raised by latest WPCS Update composer packages; Bump PHP to 7.4 and WP to 5.3 Sep 6, 2023
@thelovekesh thelovekesh marked this pull request as ready for review September 7, 2023 16:46
@@ -425,9 +426,21 @@ public function test_get_taxonomy_links() {
);
}

$post = self::factory()->post->create();

// `wp_pattern_category` is a special taxonomy that is registered for categorizing patterns.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added in recent GB release - WordPress/gutenberg#53163

@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2023

Plugin builds for 3eb015a are ready 🛎️!

@westonruter westonruter added this to the v2.4.3 milestone Sep 7, 2023
// `wp_pattern_category` is a special taxonomy that is registered for categorizing patterns.
// It is only associated with the `wp_block` post type, so we need to create a `wp_block` post.
if ( 'wp_pattern_category' === $taxonomy ) {
$post = self::factory()->post->create(
Copy link
Member

Choose a reason for hiding this comment

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

This post is overriding the $post created just before. Should the above $post creation be put in an else?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in 9b700ff

@@ -1215,8 +1212,10 @@ function ( $sources ) {

'wp_editor_has_scripts_attributed' => [
function () {
if ( version_compare( get_bloginfo( 'version' ), '5.2', '<=' ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Strange that 5.2 was the version here when 5.5 is the version it was introduced in.

Co-authored-by: Weston Ruter <westonruter@google.com>
@@ -307,7 +310,7 @@ private function get_posts_by_type( $post_type, $offset = null ) {
}
$query = new WP_Query( $args );

return $this->get_posts_that_support_amp( $query->posts );
return $this->get_posts_that_support_amp( $query->posts, $posts_to_exclude );
Copy link
Member

Choose a reason for hiding this comment

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

Instead of updating the get_posts_that_support_amp method with this new parameter, it would seem better to me to just do the filtering here in this method:

Suggested change
return $this->get_posts_that_support_amp( $query->posts, $posts_to_exclude );
$posts = array_values(
array_filter(
$query->posts,
static function ( $post ) use ( $posts_to_exclude ) {
return ! in_array( $post->ID, $posts_to_exclude, true );
}
)
);
return $this->get_posts_that_support_amp( $posts );

Copy link
Member

Choose a reason for hiding this comment

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

Hold on, this can be simplified further since we're getting IDs:

Suggested change
return $this->get_posts_that_support_amp( $query->posts, $posts_to_exclude );
return $this->get_posts_that_support_amp( array_diff( $query->posts, $posts_to_exclude ) );

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought since we already filter these post IDs and array_diff is considered slow, completing the task in a single run would be performant. However, any performance impact should be negligible here as the array size will be 100 only.

Copy link
Member

Choose a reason for hiding this comment

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

So we can go with array_diff() instead? It's less code and doens't change the resulting API.

@westonruter
Copy link
Member

I just had a concerning thought that this would cause problems for Web Stories. But I see that it already requires PHP 7.4. So we're all good there.

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.

Awesome!

@westonruter westonruter merged commit 828f229 into develop Sep 11, 2023
37 checks passed
@westonruter westonruter deleted the update/composer-packages branch September 11, 2023 21:06
@thelovekesh thelovekesh mentioned this pull request Oct 11, 2023
2 tasks
@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelogged Whether the issue/PR has been added to release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants