jetpack icon indicating copy to clipboard operation
jetpack copied to clipboard
verified publisher icon Automattic

Classic Theme Helper: Requiring the Responsive Video package files, and modifying Featured Content require process

Open coder-karen opened this issue 1 year ago • 7 comments

Fixes https://github.com/Automattic/vulcan/issues/345 Fixes https://github.com/Automattic/vulcan/issues/346

Proposed changes:

  • The intention of this PR was to require Responsive Videos from the Classic Theme Helper package in Jetpack, the Classic Theme helper plugin, and the Jetpack Mu WPcom plugin. In the process changes were made which affected how Featured Content files were required, so those changes will also need testing here.
  • Specifically, this PR updates the Classic Theme Helper package's Main class to require both the responsive-videos.php file and the featured-content.php file (removing the plugins_loaded action as this wasn't running previously).
  • The Featured_Content setup function is called directly from the Featured Content file within the Classic Theme Helper package. ~Now that file is being required, the setup function doesn't need to be called independently - except for in the jetpack-mu-wpcom package, specifically for Simple sites who do not pick this change up otherwise.~ The Featured_Content setup function is also called directly anywhere where it is needed (the jetpack-mu-wpcom package, ad currently the Jetpack plugin though that will be removed after a short period of time)

What could be changed:

  • ~Any Classic Helper Theme file that is a class could be called independently (not included in the package's Main class init function)~ Based on the review, we'll follow that (hence calling the Featured_Content setup function wherever that class is needed, instead of requiring the file).

Noting that we're not loading theme tools compat files from the package just yet as there is other functionality included in the compat files, so seems best to wait until all else is moved. Also we should be able to delete the mu-plugins version here: wp-content/mu-plugins/responsive-videos.php - or require the package version - but will wait until of these changes have made their way to WPcom and follow up then.

Other information:

  • [ ] Have you written new tests for your changes, if applicable?
  • [x] Have you checked the E2E test CI results, and verified that your changes do not break them?
  • [x] Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Jetpack product discussion

https://github.com/Automattic/vulcan/issues/345

Does this pull request change what data or activity we track or use?

No.

Testing instructions:

Self-hosted / Atomic:

To test existing behaviour:

  • For responsive videos, first ensure you are using a theme that has added support for responsive videos (for example Twenty Fifteen or Twenty Nineteen).
  • Then, create a post or page and embed a video. I found that the best way to ensure I was using responsive videos was to use the VideoPress block (which requires enabling VideoPress at Settings > Performance > Media: /wp-admin/admin.php?page=jetpack#/performance, so you'll need a WordPress.com connected site to test that)
  • View the post / page source for the published post / page. Notice the jetpack-responsive-videos-js script in the page source (and that it's source is from the jetpack plugin (or jetpack-dev if using the beta tester plugin) ), and that the video is wrapped in a div with the class jetpack-video-wrapper.
  • For Featured Content, with a theme that supports Featured Content (Dara is an example), open the Customizer. You should see a 'Featured Content' section. When opening the page source from here, you should see the featured-content-suggest-js (and that it's source on Atomic is from the jetpack-mu-wpcom-plugin plugin (or jetpack-mu-wpcom-plugin-dev if using the beta tester plugin), or jetpack / jetpack-dev on self-hosted sites ).
  • Confirm everything works as it should: Add a tag and click save, then apply that tag to a few posts - the featured content should display on the homepage.

To test this PR:

  • Apply the PR. If on a JN site make sure to apply the PR for both Jetpack and WordPress.com Features plugins. If building locally, run jetpack build plugins/jetpack, jetpack build packages/jetpack-mu-wpcom, and jetpack build plugins/mu-wpcom-plugin
  • For responsive videos, follow the steps above, then view the post / page source for the published post / page. Notice the jetpack-responsive-videos-js script in the page source (and that it's source is from the jetpack-mu-wpcom-plugin on Atomic and jetpack on self-hosted sites ) , and that the video is wrapped in a div with the class jetpack-video-wrapper.
  • For the Featured Content changes in this PR, follow the same testing steps as when testing existing behaviour.
  • Everything should test as it did before (and the featured-content-suggest-js script source is the same as before as well).

On Simple:

To test existing behaviour:

  • For responsive videos, follow the same steps as with self hosted sites (VideoPress should be enabled by default, if using the VideoPress block).
  • Notice the responsive-videos.min.js script is showing in page source, via the jetpack plugin, and that the video is wrapped in a div with the class jetpack-video-wrapper.
  • Featured Content can be tested as with self-hosted / Atomic.

To test this PR:

  • Follow the steps to apply the changes on a Simple site from the generated comment below.
  • Follow the same steps as above - notice the jetpack-responsive-videos-js script shows in page source with the jetpack-mu-wpcom-plugin as it's source (and that the video is wrapped in a div with the class jetpack-video-wrapper).
  • For Featured Content, follow the same steps as with self-hosted / Atomic. Everything should test as it did before (and the featured-content-suggest-js script source is the same as before as well).

Note that for both Simple and Atomic / Self-hosted, you can confirm that we are not relying on the Jetpack plugin to initialize the classic-theme-helper packages by modifying code directly (comment out 'theme-tools/featured-content.php', and 'theme-tools/responsive-videos.php', in projects/plugins/jetpack/modules/module-extras.php).

Noting that I've removed testing instructions for the Classic Theme Helper plugin as it looks like more work may now be needed: https://github.com/Automattic/vulcan/issues/384

coder-karen avatar Jun 20 '24 16:06 coder-karen

Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.

  • To test on WoA, go to the Plugins menu on a WordPress.com Simple site. Click on the "Upload" button and follow the upgrade flow to be able to upload, install, and activate the Jetpack Beta plugin. Once the plugin is active, go to Jetpack > Jetpack Beta, select your plugin, and enable the update/require-classic-theme-helper-package branch.

    • For jetpack-mu-wpcom changes, also add define( 'JETPACK_MU_WPCOM_LOAD_VIA_BETA_PLUGIN', true ); to your wp-config.php file.

  • To test on Simple, run the following command on your sandbox:

    bin/jetpack-downloader test jetpack update/require-classic-theme-helper-package

    bin/jetpack-downloader test jetpack-mu-wpcom-plugin update/require-classic-theme-helper-package

Interested in more tips and information?

  • In your local development environment, use the jetpack rsync command to sync your changes to a WoA dev blog.
  • Read more about our development workflow here: PCYsg-eg0-p2
  • Figure out when your changes will be shipped to customers here: PCYsg-eg5-p2

github-actions[bot] avatar Jun 20 '24 16:06 github-actions[bot]

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • :white_check_mark: Include a description of your PR changes.
  • :white_check_mark: Add a "[Status]" label (In Progress, Needs Team Review, ...).
  • :white_check_mark: Add testing instructions.
  • :white_check_mark: Specify whether this PR includes any changes to data or privacy.
  • :white_check_mark: Add changelog entries to affected projects

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 :robot:

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.

Once your PR is ready for review, check one last time that all required checks appearing at the bottom of this PR are passing or skipped. Then, add the "[Status] Needs Team Review" label and ask someone from your team review the code. Once reviewed, it can then be merged. If you need an extra review from someone familiar with the codebase, you can update the labels from "[Status] Needs Team Review" to "[Status] Needs Review", and in that case Jetpack Approvers will do a final review of your PR.

Jetpack plugin:

The Jetpack plugin has different release cadences depending on the platform:

  • WordPress.com Simple releases happen daily.
  • WoA releases happen weekly.
  • Releases to self-hosted sites happen monthly. The next release is scheduled for August 6, 2024 (scheduled code freeze on August 5, 2024).

If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack.

github-actions[bot] avatar Jun 20 '24 16:06 github-actions[bot]

Hi @coder-karen, many thanks for all the work and the amazing detailed instructions!!

I was able to test and things worked fine for me even in Simple (Not sure why I couldn't replicate @monsieur-z issues :confused: ). I wasn't able to do the plugin juggle though so I wasn't able to test the Classic Theme Helper plugin.

The Featured_Content setup function is called directly from the Featured Content file within the Classic Theme Helper package. Now that file is being required, the setup function doesn't need to be called independently - except for in the jetpack-mu-wpcom package, specifically for Simple sites who do not pick this change up otherwise.

In Simple we are calling the jetpack-classic-theme-helper/src/class-featured-content.php file so I'm not hundred percent it is necessary.

Any Classic Helper Theme file that is a class could be called independently (not included in the package's Main class init function)

I agree about this one, and also would like to get rid of calling the older module files from the theme-tools modules

darssen avatar Jun 28 '24 14:06 darssen

Thanks both for testing!

@monsieur-z can you share any more about your Simple site setup and how you are testing if you can still replicate? In my case for example, I use the bin//jetpack-downloader for both jetpack and jetpack-mu-wpcom-plugin. My sandboxed Simple site is using the Twenty Fifteen theme. The video is added using the VideoPress block. On the front-end, the iframe that the video is in is wrapped in a div with the class jetpack-video-wrapper.

In Simple we are calling the jetpack-classic-theme-helper/src/class-featured-content.php file so I'm not hundred percent it is necessary.

@darssen I'm currently thinking the issue is possibly related to when the code is being run instead (I haven't been able to identify exactly why at this stage though) - either way the file you mentioned is running but regardless of that I notice with my current code that I do need to call it in class-main.php, but when the plugins-loaded hook was used as it was before, it works fine on Simple.

The reason for removing the plugins-loaded hook in the class-main.php init function was because it wasn't working on Atomic in my tests specifically.

This is because I've now removed Automattic\Jetpack\Classic_Theme_Helper\Featured_Content::setup(); from featured-content.php in the Jetpack plugin modules file (it still works in Jetpack on self-hosted sites as it runs from the package directly). Since removing it there prevented it from working on Atomic, I realized that the jetpack-mu-wpcom package class-main.php file wasn't initializing the setup function properly on Atomic sites, as it seems it was relying on the Jetpack plugin for that.

I agree about this one

Awesome. Based on that and what I mentioned above, I've left the setup function call for featured content in the jetpack-mu-wpcom package file, but removed featured-content.php from class-main.php. I have had to add the setup function call back to the Jetpack plugin though for now, but only if a site isn't Atomic or WPcom.

and also would like to get rid of calling the older module files from the theme-tools modules

I believe that would be part of the cleanup later on? I've taken that approach with both Calypsoify and Responsive Videos - leaving them in Jetpack for a few months and then when removing the code in Jetpack that also removes the feature there.

coder-karen avatar Jul 01 '24 12:07 coder-karen

Thanks for the changes related to featured-content I like it better now since we have separated the different environments so when we end up removing it for self-hosted we only need to take care of the setup for the Jetpack plugin.

I did find one discrepancy with the test instructions provided when testing in Simple.

Follow the same steps as above - notice the jetpack-responsive-videos-js script shows in page source with the jetpack-mu-wpcom-plugin as it's source

This is not the case in my tests since it loads it from the jetpack plugins module. Can it be due to the responsive-videos.php mu-plugin in wpcom? We might want to require the Classic Theme Helper package similar to what we do for the featured-content mu-plugin.

darssen avatar Jul 02 '24 10:07 darssen

Thanks @darssen !

Following up separately about the Classic Theme Helper plugin testing instructions, I've removed these now as it looks like more work is needed on that plugin, so I can follow up separately. Issue: https://github.com/Automattic/vulcan/issues/384

Also noting here that we chatted in Slack about the Simple testing, so no changes will be made here: p1719916875736959-slack-C05PV073SG3

coder-karen avatar Jul 02 '24 12:07 coder-karen

Thanks both!

While investigating the Classic Theme Helper plugin I realized that I was inadvertently relying on the Classic Theme Helper package Main class being initialized via the Jetpack plugin (even though the page source shows the file coming from the jetpack-mu-wpcom plugin). What appeared to be happening was the check to see if the class exists in modules/theme-tools/responsive-videos.php was resulting in the class being initialized (the class having been added via the jetpack-mu-wpcom plugin on Atomic and Simple). So the issue was replicable by commenting out 'theme-tools/responsive-videos.php', in projects/plugins/jetpack/modules/module-extras.php), and the latest commit fixes that.

Later edit to add - however that one change would have also resulted in a fatal if wpcomsh was released before Jetpack (redeclaring function names), so will add this back in in a later PR.

Also added an extra note to the testing instructions to make sure to simulate a wpcomsh release prior to a Jetpack release on WoA and Simple.

coder-karen avatar Jul 02 '24 16:07 coder-karen