Move Simple Payments to PayPal Payments package #43413
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
github-actions
bot
added
[Block] Pay With Paypal
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
|
@jeherve, so far I've only moved and lightly modified the Simple Payments block code. I see there is also a Should I make a Thanks in advance for your feedback! |
Code Coverage SummaryCoverage changed in 3 files.
4 files are newly checked for coverage.
Full summary · PHP report · JS report If appropriate, add one of these labels to override the failing coverage check:
Covered by non-unit tests
|
|
That's going to be a multi-step process I think. Here is how I would personally go about it, but that's just my personal opinion, feel free to take a different approach :) The main entry point for the whole functionality is Once that's ported to the package, we can start requiring the package in the Jetpack plugin's And instead create a new modules/simple-payments.php file that would instantiate the class from the package.
Once that's done, you could copy the contents of Then, the next step would be to copy the actual block from Once that's all done, you'll be able to deprecate the old classes from the Jetpack plugin, and they could then be removed entirely in a few releases. At that point, you'll have a standalone package with code that can register a block, register a widget, and you'll be able to register that block or that widget from the Jetpack plugin and also from any other standalone plugin we create in the future. Let me know if that makes sense. |
|
@jeherve I'm finally getting back over to this. Thanks so much for the detailed guidance. I'm going to follow that to get this moving forward. |
@jeherve I have completed all of the above. At this point, I have tried running the PHP unit tests for Jetpack plugin just to make sure everything is working. The tests fail to run with the error: I think at this point, I would need to publish the package to Packagist as the lock file doesn't get updated if I run |
Update lock file.
|
I just realized I totally skipped a command to properly add the package to the Jetpack plugin's |
There was a problem hiding this comment.
I think at this point, I would need to publish the package to Packagist as the lock file doesn't get updated if I run composer update. I would publish using the PayPal Payments mirror repo, correct?
I took care of that in the last PR, when you introduced the package skeleton. The package lives here:
https://packagist.org/packages/automattic/jetpack-paypal-payments
UndefError PhanUndeclaredConstant Reference to undeclared constant \JETPACK__VERSION
Now that this lives in the package, you can use the package version (PACKAGE_VERSION) instead.
plugins_url
You may have to make some changes where scripts and styles are registered, since the files are no longer in the same location. Assets::register_script() can be a good replacement at times.
There was a problem hiding this comment.
I would recommend against moving the files ; I would suggest copying instead, and marking the different classes and methods that are left in the Jetpack plugin as deprecated with _deprecated_function. It will make it easier to spot anything we've left behind, and it will also ensure we do not create fatal errors on plugin update, for sites that have aggressive caching setups and that may continue to require a file that no longer exists.
There was a problem hiding this comment.
Ok. Should I revert all my commits from yesterday and re-do all the steps but instead of moving the files, just copy them over and modify both sets as needed?
For the old ones mark them with _deprecated_function, and the new files in the package make needed changes to register assets, update constants, etc etc.
| * @package automattic/jetpack | ||
| */ | ||
|
|
||
| require_once __DIR__ . '/../packages/paypal-payments/class-simple-payments.php'; |
There was a problem hiding this comment.
That won't work on production since the required package will not live in this directory in the production version of the plugin.
Since the package will be autoloaded, you do not have to worry about requiring the file.
There was a problem hiding this comment.
I've removed the require_once statement in 0169271.
That said, do I need to add a use statement to import the correct one? After removing the require_once my IDE is showing it using the Jetpack_Simple_Payments::get_instance(); from the deprecated class.
There was a problem hiding this comment.
I've added that use statement to get it to use the correct class in 0b47eca.
| @@ -34,7 +34,6 @@ | |||
| 'external-media/external-media.php', | |||
| 'plugin-search.php', | |||
| 'scan/scan.php', // Shows Jetpack Scan alerts in the admin bar if threats found. | |||
| 'simple-payments/simple-payments.php', | |||
There was a problem hiding this comment.
Here I would recommend that you replace loading the file by loading the module file, 'simple-payments.php'.
There was a problem hiding this comment.
Thanks for catching this! Updated in a042df9.
Update style and script registration
|
@jeherve I think I may have a mis-configuration in the package's phan config. It is throwing errors for things that I haven't changed in the file that weren't being flagged in the file when it was part of the Jetpack plugin. And it is complaining about undeclared classes, but if I had the namespace to them, my IDE complains that those classes don't exist and Phan still flags them. 🤔 Maybe I'm just missing a simple setting somewhere? |
|
As we discussed in Slack:
|
| use Automattic\Jetpack\Assets; | ||
| use Automattic\Jetpack\Current_Plan as Jetpack_Plan; |
There was a problem hiding this comment.
Since you're using specific packages inside the PayPal Payments package, you would need to add them as dependencies to the PayPal Payments package.
There was a problem hiding this comment.
Thanks for catching this. Addressed in ceccdd8.
| use Jetpack; | ||
| use Jetpack_Components; | ||
| use Jetpack_Currencies; |
There was a problem hiding this comment.
Those Jetpack-the-plugin specific dependencies aren't great. That means you won't be able to use that class on a site that doesn't run the Jetpack plugin.
I think we'll need to replace them by functionality available in packages, or move those classes to packages to be able to use them.
There was a problem hiding this comment.
As discussed in Slack, I've added a check in get_instance() to bail if this classes don't exist in 302ea6d.
| * Simple_Payments | ||
| */ | ||
| class Simple_Payments { | ||
| // These have to be under 20 chars because that is CPT limit. |
There was a problem hiding this comment.
This seems to be a left-over from something else.
There was a problem hiding this comment.
It was. I forgot to remove it. Removed in 83ee3d0.
| /** | ||
| * Original singleton. | ||
| * | ||
| * @todo Remove this when nothing calles getInstance anymore. | ||
| * | ||
| * @deprecated 10.8 | ||
| */ | ||
| public static function getInstance() { // phpcs:ignore WordPress.NamingConventions.ValidFunctionName.MethodNameInvalid | ||
| _deprecated_function( __METHOD__, 'Jetpack 10.7.0', 'Simple_Payments::get_instance' ); | ||
| return self::get_instance(); | ||
| } |
There was a problem hiding this comment.
Do we need this in the new class in the package?
There was a problem hiding this comment.
We don't. I removed it in 83ee3d0.
| class Simple_Payments { | ||
| // These have to be under 20 chars because that is CPT limit. | ||
|
|
||
| const PACKAGE_VERSION = '0.1.0-alpha'; |
There was a problem hiding this comment.
Can we use the PACKAGE_VERSION from the main PayPal_Payments class here?
There was a problem hiding this comment.
Yes we should do that. Thanks for catching this. Addressed in 83ee3d0.
| } | ||
|
|
||
| return ( ( defined( 'IS_WPCOM' ) && IS_WPCOM ) | ||
| || Jetpack::is_connection_ready() ) |
There was a problem hiding this comment.
You should be able to switch to relying on the Connection package for this (and thus require the connection package in the PayPal Payments package).
| $data['id'] = $attrs['id']; | ||
|
|
||
| if ( ! $this->is_enabled_jetpack_simple_payments() ) { | ||
| if ( jetpack_is_frontend() ) { |
There was a problem hiding this comment.
this is a Jetpack-the-plugin only function. I think you'll need to move it to a package to start using it here.
There was a problem hiding this comment.
I've added a check in get_instance() for this function. If it doesn't exist it returns null. Added in 302ea6d.
Update PACKAGE_VERSION value Remove old deprecated method
|
Copying the Simple Payments block code to the package in this commit is the cause of unrelated extensions' JS tests to fail. I haven't been able to track down the root cause. |
There was a problem hiding this comment.
This should be removed. It's an npm lockfile, and we use pnpm.
There was a problem hiding this comment.
Thanks for flagging it. I probably accidentally ran npm at some point instead of pnpm. 😅
There was a problem hiding this comment.
Removed in d12889d.
I got some assistance in Slack (p1747425409956389/1746628203.330399-slack-CDLH4C1UZ) to figure out my error. Some of the dependencies in the package didn't match the dependencies in the the Jetpack plugin. This caused issues. I've addressed it in ff0eb51. Tests are passing for me locally now. |
Fixes PAYPAL-13
Proposed changes:
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions: