Skip to content

Bump WordPress and PHP minimum#917

Merged
dkotter merged 29 commits into
developfrom
fix/requirement-bump
Aug 23, 2022
Merged

Bump WordPress and PHP minimum#917
dkotter merged 29 commits into
developfrom
fix/requirement-bump

Conversation

@peterwilsoncc

@peterwilsoncc peterwilsoncc commented Aug 3, 2022

Copy link
Copy Markdown
Collaborator

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.

Comment thread distributor.php
* 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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' );

@peterwilsoncc peterwilsoncc Aug 17, 2022

Copy link
Copy Markdown
Collaborator Author

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

@iamdharmesh iamdharmesh left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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

Comment thread includes/bootstrap.php Outdated
Co-authored-by: Dharmesh Patel <dspatel44@gmail.com>
@peterwilsoncc

Copy link
Copy Markdown
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

@iamdharmesh iamdharmesh left a comment

Copy link
Copy Markdown
Member

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.

Comment thread README.md Outdated
dkotter
dkotter previously approved these changes Aug 23, 2022
Co-authored-by: Darin Kotter <darin.kotter@gmail.com>
@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