Check for each module to only load it if the corresponding standalone plugin is not loaded

[mukeshpanchal27]

Summary

Fixes #666

Relevant technical choices

The PR changes only work when the standalone plugin is activated, and we try to activate the Performance Lab plugin. The reverse process is not working, as it generates a fatal error for a redeclare function error because all the functions are duplicated in the PL module.

Checklist

• PR has either [Focus] or Infrastructure label.
• PR has a [Type] label.
• PR has a milestone or the no milestone label.

[felixarntz]

@mukeshpanchal27 I have a suggestion here how we can make this more resilient and also refine our perflab_can_load_module() function to have it provide more context on why a module cannot load.

[felixarntz]

@mukeshpanchal27 This PR now includes a bunch of changes that are originally from the other PR #674. We should instead refresh the feature branch from trunk where that PR was merged into. I opened #676 for that, please review.

Let's merge that PR first, then you can refresh your PR here against the latest feature branch so that the PR here is "clean" again.

[felixarntz]

@mukeshpanchal27 Now that the feature branch is refreshed with trunk, can you update this branch and fix the one merge conflict?

[felixarntz]

@mukeshpanchal27 A few last things here, but functionally this already works as expected. Great work!

[joemcgill]

I personally don't like the idea of overloading this function to return an error if the module is already loaded via a standalone plugin. Instead, this function should return false if the module code is already loaded.

Here's what I would propose:

1. Create a new function, like perflab_is_module_standalone( string $slug ): bool {} that checks whether the module slug's constant is already defined.
2. Use this function inside perflab_can_load_module() to decide whether to return false in place of the current logic that throws a WP_Error.
3. Replace the else condition in perflab_render_modules_page_field that looks for WP_Errors with a new condition that checks for a false response from perflab_is_module_standalone and displays a message that says the module can't be activated because the code is being loaded by a separate plugin.

[joemcgill]

Another concern I have is that we are also using perflab_can_load_module() in perflab_is_valid_module() to determine if we should display the module as generator content in perflab_get_generator_content(). Wouldn't we still want to show that a feature is active on a site regardless of whether it's loaded as a Perf Lab module or a standalone plugin?

[felixarntz]

I'm potentially okay not returning a WP_Error, but then I also think we shouldn't intertwine the two functions at all. I think a simple boolean lacks the necessary context we would want to provide in end-user facing messaging, which is why I suggested the WP_Error usage here.

Alternatively, we can always use only bools, but then it should be individual functions that are individually called, and I don't think we should put the perflab_is_module_standalone() call into perflab_can_load_module() as otherwise the context is again intertwined and the return value does not convey why.

While the above might be okay, I still think even with that it has drawbacks compared to using a WP_Error: When you look at our admin/load.php file, you see all those manual messages in there for SQLite, which with a WP_Error return value we could work towards avoiding those. Those messages should rather be located where those problems are determined, not manually be re-checked when we already have that logic somewhere else. I think using false instead of WP_Error lacks context and will require us to repeat code unnecessarily. What is the concern about returning a WP_Error?

[joemcgill]

What is the concern about returning a WP_Error?

I have two main concerns:

1. It complicates the function signature and purpose unnecessarily and requires any code using this function to check for a possible error message OR a boolean value, when the intent of the function is mainly to determine if a module can be loaded. Keeping the function simple makes it easier to reason about and easier to test to avoid future bugs.
2. WP_Error messages should be used when we actually want to relay that an error has occurred. In this case, we're using it as a logical value in our code so that we can avoid an error. That seems wrong.

I'd be happy to decouple all of this logic as well, if you'd prefer, I just think it would be weird for a function called perflab_can_load_module() to return true if we know that the code has already been loaded, which is why I'm proposing the approach I suggested.

[felixarntz]

@joemcgill Your rationale for 1. and 2. makes sense. However we still need to think through how we can solve that alternatively in a way that also works and doesn't lose context. The purposes of the two calls are different:

• When we load the modules, we just want to know whether they can be loaded or not (which encompasses both checking a potential can-load.php file for the module and checking whether its code is already loaded by the standalone plugin). In that case true or false works well.
• When we show the WP admin screen, we want to disable the checkboxes for modules that cannot be loaded, and we also want to show an informative message for why they're not loaded. For that purpose, having only true or false is not enough.

I'd be happy to decouple all of this logic as well, if you'd prefer, I just think it would be weird for a function called perflab_can_load_module() to return true if we know that the code has already been loaded, which is why I'm proposing the approach I suggested.

I'm not sure I understand the last part of what you mean here. As far as I can tell, we wouldn't know whether the module code was already loaded on the WP Admin screen: For example, if the module is not active, this function wouldn't have been called anytime before in the same request. Or when the module is active, we don't know if the code is loaded because of the module or because of its standalone plugin. That's why I think we need to call that function on the WP admin screen as well.

We could avoid the usage of WP_Error by having two separate functions perflab_can_load_module() (responsible for running the can-load.php logic) and perflab_is_module_standalone() (responsible for checking whether the module's standalone plugin is already loaded), and we would need to call both of them in both places. For the WP admin screen, we could then show different messages based on which function returns false.

That still keeps us limited though in providing more helpful messages for the can-load.php approach itself, the lack of which right now causes us to have this messy SQLite module specific logic hard-coded in the general WP admin screen code, just to show better messages based on duplicated logic. So I would like us to figure that one out before we discard the WP_Error approach.

Again, I agree the WP_Error approach is probably not the best approach, but we also shouldn't settle for an approach that lacks the flexibility that we're looking for.

[mukeshpanchal27]

Thanks @joemcgill and @felixarntz for the feedback. It look promising to me and I implemented it in 0c5bc48. It removes the WP_Error and introduce new function perflab_is_module_standalone as suggested.

Can we open a new issue and work on SQLite module stuff if it isn't a blocker for this issue? 

[felixarntz]

Thanks @mukeshpanchal27, a new follow up issue sounds like a great idea. We can use that issue to implement a better architecture for handling why a module cannot be loaded and making this information available to the end user.

The approach you implemented looks good, I just left a review in #670 (review).

[mukeshpanchal27]

Thanks @felixarntz and @joemcgill for all your feedback and thoughts.

At the moment, I'm facing issues with the constant approach.

• Take a pull from this PR and spin up the ENV and check the plugin admin page( Admin Panel > Settings > Performance). You can see that the WebP Uploads module is disabled with the message "The module cannot be managed with Performance Lab since it is already active as a standalone plugin." Make sure only the PL plugin is activated.

It is because the Pl plugin loads all the modules in "load.php" in the "plugins_loaded action, so "WEBP_UPLOADS_VERSION is already defined for the admin screen.

I hope the above explanation helps to replicate the issue.

[felixarntz]

@mukeshpanchal27 Thank you for clarifying, now I see what you mean. It's because once the admin page is loaded, the constant will always be defined.

I think what we could do is add another check alongside the constant check that the constant value does not start with "Performance Lab ":

• If it starts with "Performance Lab ", it means the module is loaded.
• If it doesn't start with "Performance Lab ", it means the standalone plugin is loaded. In that case the checkbox should be disabled.

[joemcgill]

I'm curious if there's a way we could use the settings to determine whether the module is loaded from the performance lab plug-in versus being loaded from an external plug-in? Additionally if we discover that a conflict exists because the standalone plug-in is active we should disable the option from performance lab.

[mukeshpanchal27]

@felixarntz Thanks for the feedback. It solve the issue for admin screen. PR is ready for review.

[felixarntz]

@mukeshpanchal27 This is pretty much good to go from my perspective, just two tiny things here. More importantly though, I am not yet marking this as approved since we first have to figure out what to do regarding the WP_Error usage (see #670 (comment)).

[felixarntz]

@mukeshpanchal27 Almost LGTM, I am suggesting two small adjustments.

[felixarntz]

LGTM, thanks @mukeshpanchal27!

Just two suggestions to clarify doc comments, which I can commit.

[felixarntz]

@joemcgill Can you give this another pass please?

@mukeshpanchal27 Could you open an issue to further explore #670 (comment)? This can wait, but we should log it for the future.

[joemcgill]

This is looking ok to me now. I left one question about how we plan to handle changing the constant value, but that can be handled outside this PR.