-
Notifications
You must be signed in to change notification settings - Fork 7
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
Add Compat Checker library for versioning and compatibility checks #79
Conversation
I've added caching for |
@mikkamp, @budzanowski - Resolved versioning issues by adding the version information to the namespace. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😮 This is very cool! Thank you for the extremely detailed testing instructions. The tests worked well with Brands and Product Filters, although I did discover that I had to delete the transients repeatedly once I started changing the versions in the header.
I added a handful of comments, mostly about links and notice settings.
|
||
$message = sprintf( | ||
/* translators: %1$s - Plugin Name, %2$s - Plugin version, %3$s - WooCommerce version number */ | ||
esc_html__( '%1$s - %2$s is untested with WooCommerce %3$s.', 'woogrow-compat-checker' ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Should the merchant be able to dismiss this error? Otherwise it just hangs out until the plugin is updated to support my WooCommerce version (good thing always on top of compatibility releases!!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made the warning notice dismissible in this commit: c8c50e0
The is-dismissible
applies only on the current screen. To maintain the notice state, we'll have to store the notice state in the user meta data. Do you want me to implement that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm ... I guess for WooCommerce it's not particularly critical because our extensions should always be up to date with WooCommerce. We could either make it dismissible until the next WooCommerce update, or leave as is and then change it if we see that our extensions are falling behind on compatibility (+1 technical debt)
Thank you, @layoutd, for taking the time to review this PR. I'll go through your feedback and shall add my comments/fixes. |
Hello again, @layoutd. Thank you for taking the time to review this PR. I've made the changes you suggested. This is ready for another round of review. I've also added a question related to the dismiss notice. Please let me know what you think. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - the changes work well and I think this is OK as is. I still have a few more comments though (😳)
- There's a bit of a hiccup in the "Install WooCommerce" link process, where the prompt to install the plugin appears on the page where I just installed the plugin:
I wonder if that check could be skipped on that page in particular? But it's not a big deal since it ends up resolving itself, just could be a bit confusing. - I added some notes to the testing instructions as a reminder that that the compatibility transient needs to be erased for version testing.
- I wonder if this text should just be
WooCommerce Brands version 1.6.58 …
orWooCommerce Brands - version 1.6.58 …
.
I agree. I'll fix this.
Thank you.
I'd go with |
Hey @layoutd, I've addressed the below comment in a8579b7:
In e7bf974, I've set the text in the notice like this: I'd like you to take a look at these two changes and let me know your feedback. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Both "issues" look like they've been solved, thanks for humoring me @ibndawood!
Thank you, @layoutd, for your time to review the final changes. I'll merge this PR to the |
Changes proposed in this Pull Request:
Closes #75.
This PR:
PHP
package calledcompat-checker
that can be installed via composer in WooCommerce Grow extensions.Detailed test instructions:
Setting up the fork.
Copy the trunk branch only
is unchecked.Create Fork
button. This step will create a fork ofwoocommerce/grow
. Let's say:ibndawood/grow
add/compat-checker
branch intotrunk
.Testing the GitHub Workflow
Actions
tab in the forked repository.Public Compat Checker package
workflow is added.compat-checker
.trunk
to trigger the workflow.packages/php/compat-checker
contents are published into thecompat-checker
branch.Including the library in the WooCommerce Extension
compat-checker
package by adding the repository to thecomposer.json
of the cloned repository like in the snippet below. Replacehttps://github.com/ibndawood/grow
with the URL of your fork:composer update
to includedwoocommerce/grow
in the plugin'svendor
folder.vendor
folder has thewoocommerce/grow
folder.vendor/composer/autoload_psr4.php
file hasAutomattic\WooCommerce\Grow\Tools\
namespace included.Testing compatibility and versioning
Setup
In the main plugin file, run
Automattic\WooCommerce\Grow\Tools\CompatChecker\v0_0_1\Checker::instance()->is_compatible( __FILE__, $plugin_version )
to check for compatibility and versioning. The snippet should be placed before initialising the plugin. The below example snippet is for WooCommerce Brands:WooCommerce activation tests
With the above code snippets in place, run the following tests. The instructions are for WooCommerce Brands but can be used for other WooGrow Extensions.
WooCommerce Brands requires WooCommerce to be installed and activated. Please install WooCommerce.
is displayed.WooCommerce Brands requires WooCommerce to be activated. Please activate WooCommerce.
is displayed.WooCommerce versioning tests
_transient_wc_grow_compat_checker_[EXTENSION].php[VERSION
after each change!!WC requires at least
header field and set it to a version higher than the active WooCommerce version. Let's say theWC requires at least
is set to8.2
and the active WooCommerce version is8.0.2
.WooCommerce Brands requires WooCommerce version 8.2 or higher. Please update WooCommerce to the latest version, or download the minimum required version
is displayed.WC tested up to
and set it to a lower version than the active WooCommerce version. Let's say theWC tested up to
is set to7.8
, the active WooCommerce version is8.0.2
and WooCommerce Brands version is1.6.56
.WooCommerce Brands - 1.6.56 is untested with WooCommerce 8.0.2
is displayed.L2 support recommendation notice
7.6
.Heads up! WooCommerce Brands will soon discontinue support for WooCommerce 7.6. Please update WooCommerce to take advantage of the latest updates and features.
WordPress versioning tests
_transient_wc_grow_compat_checker_[EXTENSION].php[VERSION
after each change!!Tested up to
and set it to a lower version than the current WordPress version. Let's say theTested up to
is set to6.2
, the active WordPress version is6.3
, and the WooCommerce Brands version is1.6.56
.WooCommerce Brands - 1.6.56 is untested with WordPress 6.3
is displayed.Changelog entry