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

Bump WordPress and PHP minimum #917

Merged
merged 29 commits into from
Aug 23, 2022
Merged

Bump WordPress and PHP minimum #917

merged 29 commits into from
Aug 23, 2022

Conversation

peterwilsoncc
Copy link
Collaborator

@peterwilsoncc peterwilsoncc commented Aug 3, 2022

Description of the Change

Bump WordPress minimum to 5.7
Bump PHP minimum to 7.4

Closes #905, Closes #925

How to test the Change

Changelog Entry

Changed - Now requires PHP 7.4 or later and WordPress 5.7 or later.

Credits

Props @peterwilsoncc, @vikrampm1, @iamdharmesh.

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@@ -55,14 +55,29 @@ function( $class ) {
);

/**
* Require PHP version 5.6 - throw an error if the plugin is activated on an older version.
* Require PHP 7.4+, WP 5.7+ - throw an error if the plugin is activated on an older version.
*/
register_activation_hook(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These changes prevent activation of the plugin on unsupported versions of WP, PHP.

There is now the small risk a site will have an active plugin and update to a version it no longer supports. How have we handled that in the past?

Copy link
Collaborator

Choose a reason for hiding this comment

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

So I was trying to find a version of this in our plugins and couldn't quite remember where it's at but what we've done in the past is have some code in the main plugin file (so this file in this case) that checks if our requirements are met and if not, output an admin notice and stop execution of the plugin. Not our plugin but here's a similar approach in a Woo plugin: https://github.com/woocommerce/google-listings-and-ads/blob/develop/google-listings-and-ads.php#L41 and https://github.com/woocommerce/google-listings-and-ads/blob/develop/src/Internal/Requirements/VersionValidator.php#L22

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In 6187c56 I've added a first pass at showing a notice if the plugin fails to meet minimum version requirements.

Honestly, it seems like a lot for something that ought to be quite minor but I am not sure if there is much that can be done about that. For this to work, the plugin's main file will always need to be compatible with earlier versions of WP/PHP. The up-to-date code will need to come later.

Should I move some of the bootstrapping code so distributor.php can focus on activation & a bootstrapper can focus on bootstrapping?

Comment on lines -29 to +32
$post = isset( $_GET['post'] ) ? get_post( (int) $_GET['post'] ) : false; // @codingStandardsIgnoreLine Nonce not required

if ( $post && ! \Distributor\Utils\is_using_gutenberg( $post ) ) {
add_action( 'do_meta_boxes', __NAMESPACE__ . '\replace_revisions_meta_box', 10, 3 );
add_action( 'add_meta_boxes', __NAMESPACE__ . '\add_revisions_meta_box' );
}
add_action( 'do_meta_boxes', __NAMESPACE__ . '\replace_revisions_meta_box', 10, 3 );
add_action( 'add_meta_boxes', __NAMESPACE__ . '\add_revisions_meta_box' );
Copy link
Collaborator Author

@peterwilsoncc peterwilsoncc Aug 17, 2022

Choose a reason for hiding this comment

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

The is_using_gutenberg() function call is moved to the do_meta_boxes hook where it is used. This ensures all the editor functions are defined before attempting to use them. Specifically, in this instance, use_block_editor_for_post_type().

Instead of reproducing the function (including the final filter) call the function if it exists. Otherwise go through the polyfill steps.
@peterwilsoncc peterwilsoncc marked this pull request as ready for review August 18, 2022 05:46
@peterwilsoncc peterwilsoncc requested review from a team and iamdharmesh and removed request for a team August 18, 2022 05:46
@jeffpaul jeffpaul added this to the 2.0.0 milestone Aug 18, 2022
Copy link
Member

@iamdharmesh iamdharmesh left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the great work here @peterwilsoncc, Code looks good to me just want you to check on the notes below.

  • I am getting 404 for admin.min.css
    image

  • I think we should also add minimum requirements in the plugin header of the plugin’s main PHP file as well. Since WordPress 5.8 plugin readme files are not parsed for requirements. This means that headers Requires PHP and Requires at least are going to be parsed from plugin’s main PHP file.

Thanks

@peterwilsoncc
Copy link
Collaborator Author

@iamdharmesh I've added the requirements to the main plugin file in c1c93fb

You'll need to run npm i; npm run build in your command line for the css files to be generated.

@iamdharmesh
Copy link
Member

Thanks for adding requirements to the main plugin file @peterwilsoncc.

You'll need to run npm i; npm run build in your command line for the css files to be generated.

Yes, I am already doing this, but it seems the URL to load CSS file is wrong. it is .../distributor/includes/dist/css/... instead of .../distributor/dist/css/....

Thanks

includes/bootstrap.php Outdated Show resolved Hide resolved
Co-authored-by: Dharmesh Patel <[email protected]>
@peterwilsoncc
Copy link
Collaborator Author

it is .../distributor/includes/dist/css/... instead of .../distributor/dist/css/....

Sorry, I missed that in the screen shot.

I've merged in your chagne

iamdharmesh
iamdharmesh previously approved these changes Aug 22, 2022
Copy link
Member

@iamdharmesh iamdharmesh left a comment

Choose a reason for hiding this comment

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

Thanks for the changes @peterwilsoncc. All looks good to me now. 🚀

Thanks for your amazing work here.

README.md Outdated Show resolved Hide resolved
dkotter
dkotter previously approved these changes Aug 23, 2022
@peterwilsoncc peterwilsoncc dismissed stale reviews from dkotter and iamdharmesh via e3c3999 August 23, 2022 22:35
@dkotter dkotter merged commit 014c3c9 into develop Aug 23, 2022
@dkotter dkotter deleted the fix/requirement-bump branch August 23, 2022 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Remove application passwords dependency Bump WordPress and PHP minimums
4 participants