Masterbar: Copy module code to package

[fgiannar]

This PR copies the Masterbar module codebase to the masterbar package. In the process:

• Methods that only live in the Jetpack plugin have been replaced by corresponding methods available in packages. For example: Jetpack::is_module_active has been replaced by ( new Modules() )->is_module_active
• The JETPACK__VERSION used for cache busting when enqueueing scripts and styles is now replaced by Main::PACKAGE_VERSION
• Translations text domain is updated to jetpack-masterbar from jetpack
• Updated the build process so that rtl files are not generated with a custom config (we used to refer to them as weird RTL entries because -rtl was appended instead of .rtl) and the corresponding code references as well. Not sure however, if we should still create them by appending -rtl so that wp picks up the rtl property and auto loads them?
• Updated all scripts and css files to be loaded from the dist folder

Other information:

• [ ] Have you written new tests for your changes, if applicable?
• [ ] Have you checked the E2E test CI results, and verified that your changes do not break them?
• [ ] 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

pfwV0U-3U-p2

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

No

Testing instructions:

• Code Review since the package is not used anywhere yet.
• Migrated unit tests should all pass
• Try running jetpack build packages/masterbar and inspect the generated assets in the build folder

[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.

[github-actions[bot]]

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/copy-masterbar-code-to-pkg 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/copy-masterbar-code-to-pkg
  

  bin/jetpack-downloader test jetpack-mu-wpcom-plugin update/copy-masterbar-code-to-pkg
  

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

[fgiannar]

@Automattic/jetpack-vulcan How do we usually handle the ESLint failing tests related to missing JSDoc elements? Should I manually fix those or add an ignore rule?

[fgiannar]

Add eslint-disable rules related to JSDoc requires

@Automattic/jetpack-vulcan How do we usually handle the ESLint failing tests related to missing JSDoc elements? Should I manually fix those or add an ignore rule?

FYI I added eslint-disable rules via https://github.com/Automattic/jetpack/pull/37342/commits/cae099fb1d315550c0ca1a5fa11337e198477c53 and https://github.com/Automattic/jetpack/pull/37342/commits/513f7a572b785b048bfeb6960e90ce83edf28068

[coder-karen]

I've not looked through the full list of phan flagged issues, but certainly some look like false positives, eg Error: UndefError PhanUndeclaredClassConstant Reference to constant MANAGE_PLUGINS from undeclared class \WPCOM_Features (as it's a check to see if it exists). If nothing else stands out as being fixable, if you add a baseline.php file then run jetpack phan --update-baseline packages/masterbar it will create the phan baseline file and add all the relevant suppressions.

[fgiannar]

I've not looked through the full list of phan flagged issues, but certainly some look like false positives, eg Error: UndefError PhanUndeclaredClassConstant Reference to constant MANAGE_PLUGINS from undeclared class \WPCOM_Features (as it's a check to see if it exists). If nothing else stands out as being fixable, if you add a baseline.php file then run jetpack phan --update-baseline packages/masterbar it will create the phan baseline file and add all the relevant suppressions.

Thanks Karen! I moved the PR back to In progress as I'd like to solve all the issues phan flagged :)

That said, if you have any suggestions around Brad's suggestion ( using the Assets package for loading styles vs keeping the webconfig as is and load styles without using Assets ) pls let me know. Relevant internal discussion: p1715693085327219-slack-C05PV073SG3

[anomiex]

Error: UndefError PhanUndeclaredClassConstant Reference to constant MANAGE_PLUGINS from undeclared class \WPCOM_Features (as it's a check to see if it exists).

This particular one should be fixed by updating projects/packages/masterbar/.phan/config.php to load the wpcom stubs when processing the package, which would look something like this:

return make_phan_config( dirname( __DIR__ ), array( '+stubs' => array( 'wpcom' ) ) );

[anomiex]

Taking a glance at some of the other issues Phan is reporting,

UndefError PhanUndeclaredFunctionInCallable Call to undeclared function amp_add_customizer_link in callable

Same as the wpcom stubs mentioned above, adding 'amp' to the array too.

UndefError PhanUndeclaredClassMethod Call to method is_module_active from undeclared class \Jetpack

I'm going to be making a pass over these across the monorepo soon. When it's something in another plugin, like Jetpack-the-plugin here, my plan is to add into the package's Phan config something like this in the $options array passed to make_phan_config().

'parse_file_list' => array(
    /* Reference files to handle code checking for stuff from Jetpack-the-plugin or other in-monorepo plugins.
     * Wherever feasible we should really clean up this sort of thing instead of adding stuff here.
     * 
     * DO NOT add references to files in other packages like this! Generally packages should be listed in composer.json 'require'.
     * If there are truly optional dependencies or circular dependencies that can't be cleaned up, one package may list the
     * other in 'require-dev' and `extra.dependencies.test-only' instead. See packages/config for an example.
     */
    __DIR__ . '/../../../plugins/jetpack/class.jetpack.php',
),

As the note mentions, don't do that for cross-package things though.

UndefError PhanUndeclaredFunction Call to undeclared function \wpcomsh_is_site_sticker_active()

Probably just suppress this for now, pending pf4qpu-nc-p2.

TypeError PhanTypePossiblyInvalidDimOffset Possibly invalid offset 0 of $menu_item of array type array{0?:non-empty-string,3?:string,1?:non-empty-string,2?:non-empty-string,4?:'menu-top',6?:non-empty-string}|non-empty-mixed

This is a tricky one. This particular example is being raised on line 167 of projects/packages/masterbar/src/admin-menu/class-base-admin-menu.php. Adding '@phan-debug-var $menu_item'; temporarily near line 131 gives two outputs:

131 | CommentError PhanDebugAnnotation @phan-debug-var requested for variable $menu_item - it has union type
    | \Countable|array{5?:string,4?:'hide-if-js'|string,6?:'none'}|non-empty-array<mixed,mixed>|non-empty-mixed(real=non-empty-mixed)
131 | CommentError PhanDebugAnnotation @phan-debug-var requested for variable $menu_item - it has union type non-empty-mixed(real=non-empty-mixed)

We have Phan configured to do two passes. The first pass generated the second of those two messages, all it knows is that items in the $menu global are "mixed", and the check just above line 131 allowed it to refine that a bit to "non-empty-mixed". The second pass collected incomplete information from some of the assignments, which lets it complain at line 167 that it doesn't know anything about an offset 0 in the array.

The real fix is probably for me to declare some of these WordPress globals with helpful types so it doesn't see "mixed" in the first place. That's on my todo list (see p1715703208406399/1715701565.543679-slack-CDLH4C1UZ) but I've had a busy week so far. 🙂

A more local fix would be to add a '@phan-var array $menu_item'; (or something with more complex array shapes using array{...} syntax) near line 131 to tell it the type is really array rather than mixed at that point.

---

Feel free to ping me with other Phan questions.

[anomiex]

The real fix is probably for me to declare some of these WordPress globals with helpful types so it doesn't see "mixed" in the first place. That's on my todo list (see p1715703208406399/1715701565.543679-slack-CDLH4C1UZ) but I've had a busy week so far. 🙂

See if #37427 helps with that one.

[fgiannar]

@anomiex Many thanks for the phan advice, it was really helpful!

I'm going to be making a pass over these across the monorepo soon. When it's something in another plugin, like Jetpack-the-plugin here, my plan is to add into the package's Phan config something like this in the $options array passed to make_phan_config().

This means that stuff like the following would not be flagged anymore, right?

UndefError PhanUndeclaredClassMethod Call to method is_module_active from undeclared class \Jetpack

However this is very helpful, since in this case Jetpack::is_module_active can be replaced with ( new Modules() )->is_active.

Here are a few issues phan flagged and I'm not sure what's the best way to handle:

NOOPError PhanNoopNew Unused result of new object creation expression in new Masterbar() (this may be called for the side effects of the non-empty constructor or destructor)

Suppress those?

UndefError PhanUndeclaredClassInCallable Reference to undeclared class \Jetpack_Custom_CSS_Enhancements in callable \Jetpack_Custom_CSS_Enhancements::customize_register

UndefError PhanUndeclaredFunction Call to undeclared function \wpcomsh_is_site_sticker_active()

Suppress those too?

UndefError PhanUndeclaredClassMethod Call to method is_amp_available from undeclared class \Jetpack_AMP_Support

I'd expect this to work after loading the amp stubs

TypeError PhanTypeMismatchArgumentNullableInternal Argument 1 ($filename) is JETPACK__GLOTPRESS_LOCALES_PATH of type ?array|?bool|?float|?int|?resource|?string but \file_exists() takes string (expected type to be non-nullable)

UndefError PhanUndeclaredClassInstanceof Checking instanceof against undeclared class \GP_Locale

Again I could suppress those but I wanted to verify that's the recommended way to go here :)

[anomiex]

However this is very helpful, since in this case Jetpack::is_module_active can be replaced with ( new Modules() )->is_active.

My pass will of course be for all the existing code. New code, including stuff like this that's being moved out of Jetpack into a package, would need someone to also copy the stuff into the new package's .phan/config.php. So you should still see them, at least initially. 🙂

NOOPError PhanNoopNew Unused result of new object creation expression in new Masterbar() (this may be called for the side effects of the non-empty constructor or destructor)

Ideally we wouldn't do new Whatever() for the side effects at all, we'd do like Whatever::init() or Whatever::setup_actions() or something.

Other options include using @phan-constructor-used-for-side-effects on the class as mentioned at https://github.com/phan/phan/blob/v5/internal/Issue-Types-Caught-by-Phan.md#phannoopnew.

UndefError PhanUndeclaredClassInCallable Reference to undeclared class \Jetpack_Custom_CSS_Enhancements in callable \Jetpack_Custom_CSS_Enhancements::customize_register

Assuming there's no way to replace this reference to Jetpack-the-plugin with something else, doing like I mentioned above in the .phan/config.php to reference projects/plugins/jetpack/modules/custom-css/custom-css.php would probably be the way to go.

UndefError PhanUndeclaredFunction Call to undeclared function \wpcomsh_is_site_sticker_active()

Probably just suppress this one for now, pending pf4qpu-nc-p2. Same for other functions from wpcomsh like site_is_private().

UndefError PhanUndeclaredClassMethod Call to method is_amp_available from undeclared class \Jetpack_AMP_Support

I'd expect this to work after loading the amp stubs

Yes, that one should.

TypeError PhanTypeMismatchArgumentNullableInternal Argument 1 ($filename) is JETPACK__GLOTPRESS_LOCALES_PATH of type ?array|?bool|?float|?int|?resource|?string but \file_exists() takes string (expected type to be non-nullable) UndefError PhanUndeclaredClassInstanceof Checking instanceof against undeclared class \GP_Locale

For now you'll probably want to add a dep on the compat package to get the class. Then I'm not sure what to do about the constant; at first I was going to say to let autoloading handle finding the class, but it seems the compat package doesn't actually set up autoloading which is weird.

The real fix is probably along the lines of https://github.com/Automattic/jetpack/issues/2707#issuecomment-2036701663.

[anomiex]

UndefError PhanUndeclaredClassMethod Call to method is_amp_available from undeclared class \Jetpack_AMP_Support

I'd expect this to work after loading the amp stubs

Yes, that one should.

Wait, no it won't. That's using the Jetpack_AMP_Support class from Jetpack-the-plugin, not the AMP functions from the AMP plugin.

I see three options here:

1. Add a reference to projects/plugins/jetpack/3rd-party/class.jetpack-amp-support.php as described earlier. The AMP stuff would then only work when the package is used in combination with Jetpack-the-plugin. Since the long-term here is to only have this package in jetpack-mu-wpcom, that's probably ok.
2. Have it check for and call the AMP plugin functions directly instead of going through Jetpack_AMP_Support. The 'amp' stubs would make Phan happy with that.
3. I suppose you could move the AMP support out to a separate package for this package to depend on. Be careful not to break Simple in that case by autoloading the Jetpack_AMP_Support class though, cf. #30195.

[anomiex]

However this is very helpful, since in this case Jetpack::is_module_active can be replaced with ( new Modules() )->is_active.

My pass will of course be for all the existing code. New code, including stuff like this that's being moved out of Jetpack into a package, would need someone to also copy the stuff into the new package's .phan/config.php. So you should still see them, at least initially. 🙂

Also, now that I think about it, probably the better way to find these would be to add @deprecated to the Jetpack method so Phan starts complaining about use of it. Wouldn't catch use in callables, unfortunately, but this one in particular shouldn't be called as a callable anyway.

[fgiannar]

Added a phan baseline via https://github.com/Automattic/jetpack/pull/37342/commits/30e2dd2d66d5c1ad5f843d6f94ca2455de721bda which includes all the remaining phan issues I plan to fix gradually in follow-up PRs while switching the usage to the package.

[fgiannar]

@darssen Many thanks for the review! Really appreciate the time you took!

left some comments, mainly regarding missing require_once and some inconsistencies in the @package annotation. I only pinpointed two of the different @package annotations that need to be looked at to avoid making too much noise in the PR but it would be nice to make sure all of them are properly set.

I believe these are all addressed now. I've also left responses to all your inline comments!

Also, when reviewing differences between the module and the package I found two fixes that have been done in the module that we would like to port to the package code. (https://github.com/Automattic/jetpack/pull/37314, https://github.com/Automattic/jetpack/pull/37315).

Amazing thanks! Done with https://github.com/Automattic/jetpack/pull/37342/commits/a2b5a0fa7d49224f9201bef2b5d838294cae4152

Would you mind giving this a final look?

[fgiannar]

@anomiex Since you were also involved in this PR would you mind giving it a final check too? Many thanks!

[fgiannar]

LGTM. I left some suggestions inline for further improvement, but I'd be fine with merging this and iterating.

Thanks @anomiex ! I wanted to update the README anyways and thought it would be better to implement all suggestions before merging :)