-
Notifications
You must be signed in to change notification settings - Fork 816
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
Classic Theme Helper: Load relevant theme compat functionality from package #41598
Classic Theme Helper: Load relevant theme compat functionality from package #41598
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?
|
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 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Follow this PR Review Process:
Still unsure? 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. Mu Wpcom plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Wpcomsh plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Classic Theme helper plugin plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Code Coverage SummaryCoverage changed in 1 file.
|
… to prevent loading conflicts with the Jetpack module filter which will remain.
Code Coverage SummaryCoverage changed in 1 file.
|
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.
Thanks for the work here @coder-karen!
Changes make sense. I have added a comment regarding deprecating one public method instead of directly removing it. Let me know your thoughts.
The testing went well mainly, but for WoA I was not able to get the twentyfifteen-jetpack
script file to ensure reading the module file functions that are not duplicated in the Classic Theme Helper package. Is there something I am missing?
@@ -78,15 +76,15 @@ public static function jetpack_load_theme_tools() { | |||
/** | |||
* Load theme compat file if it exists. | |||
*/ | |||
public static function jetpack_load_theme_compat() { | |||
public static function classic_theme_helper_load_theme_compat() { |
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.
Although this is the Classic Theme Helper Package and its use might be minimal outside from the monorepo, should we deprecate first since it is a public method?
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.
For the Classic Theme Helper package, we hadn't started hooking into that function yet, so it was never in use. I also searched internally and also via WPdirectory for the original function name, and it looks like no plugins or themes (or us) make use of the function whether it's via the modules file or package.
I'm reading mixed messaging about best practices in this case though.
The other option is to change the name back to what it was so as not to delay loading the features from the package, given there are also other unchanged function names in the same file.
Given that it wasn't in use, and the functionality it would have allowed was very minimal (and still duplicated in the modules file), I can't see why it would have been used externally. I'd veer toward renaming given all of that, but given the extra context where do you stand?
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.
Thanks for all the details. After considering them I agree we should be fine by renaming 👍
Thanks for testing!
I'm not too sure. What is your WoA test site setup? In my case, I have a WoA site, with the Beta tester plugin installed, and the branch checked out both via Jetpack and Jetpack Mu WPcom. I have installed and activated Twenty Fifteen, and with no ther changes I visit the homepage and open the page source, searching for |
Yes, I am able to see the same |
Fixes https://github.com/Automattic/vulcan/issues/389
Proposed changes:
modules/theme-tools.php
toclassic_theme_helper_theme_compat_files
. This is to prevent loading issues that were obvious on Simple sites without the name change due to a file that hooks into the existing filter for those sites. Renaming the filter in the package prevents conflicts and continues to allow extensibility.Other information:
Jetpack product discussion
https://github.com/Automattic/vulcan/issues/389
Does this pull request change what data or activity we track or use?
No.
Testing instructions:
Test on self-hosted (testing locally or with the beta tester plugin), or WoA (using the beta tester plugin). The changes in this PR have no bearing for Simple sites but we can test that they are not affected - so test using the commands in the generated comment below on a sandboxed Simple site. On WoA and Simple the below implies both branches are checked out - if only one on WoA this just means we'll be loading only via that branch parent, but all functionality would still work).
Some random tests to confirm functionality is now loading via the Classic Theme Helper package:
twentyfourteen_customizer_default
function including the 'function_exists' check inmodules/theme-tools/compat/twentyfourteen.php
(If you load only via the Jetpack Mu WPcom plugin when testing WoA, commenting out the module file paths will prevent functionality. )/wp-admin/admin.php?page=jetpack#performance
), and then opening up page source on the the post front-end. You should see reference tojetpack-responsive-videos-css
orresponsive-videos.css
(orresponsive-videos.js
on WoA). On Simple, the best option is to look for thejetpack-video-wrapper
class in the div wrapping the video - for Simple we just want to confirm it is still working.To confirm that we're still also reading the module file functions that are not duplicated in the Classic Theme Helper package:
twentyfifteen-jetpack-css
script. It should be the modules file path.For Twenty Fourteen, Sixteen, Nineteen, Twenty, Twenty One, it's the same -
twentyfourteen-jetpack-css
etc.twentyfifteen-jetpack
etc.