Skip to content
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

Prevent fatal error from sharing data being available for a non-existent module #5488

Closed
felixarntz opened this issue Jun 30, 2022 · 5 comments
Labels
P1 Medium priority PHP Type: Bug Something isn't working

Comments

@felixarntz
Copy link
Member

felixarntz commented Jun 30, 2022

See this Slack thread: We need to make sure that, if sharing data for a module which is no longer available is part of the sharing settings, submitting/modifying the sharing settings does not result in a fatal error. This currently happens for me consistently, because I have previously configured sharing settings for Idea Hub, but now disabled the module entirely (which can happen either through the googlesitekit_available_modules filter or in some cases feature flags).

Screenshot:
Screen Shot 2022-06-30 at 11 28 53 AM

The PHP fatal error I get is: PHP Fatal error: Uncaught Exception: Invalid module slug idea-hub. in /includes/Core/Modules/Modules.php:557


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • No fatal error must be triggered from submitting sharing settings that include data for a module that is not (realistically "no longer") available.
  • If sharing data for a non-existent module is submitted, it should be ignored.
    • In other words, if sharing data for this module was previously set in the database option, it should just be kept at what it was before. If there was none set before, it should not be set either.

Implementation Brief

  • In includes/Core/Modules/Modules.php:
    • Create a new public method named module_exists() which accepts the parameter $slug.
    • Inside the method, simply use get_module( $slug ) in a try/catch block returning true if it doesn't throw, and false if it does (similar to the is_module_connected() method).
    • Inside the array_walk callback for both 'add_option_' . Module_Sharing_Settings::OPTION and 'update_option_' . Module_Sharing_Settings::OPTION action hooks' callbacks, before getting the module data, do a check if the module exists using the new module_exists() method. Execute the remaining code only if the module exists:
      function( $value, $module_slug ) {

      function( $value, $module_slug ) use ( $old_values ) {

Test Coverage

  • No tests need to be added/updated.

QA Brief

  • Open the dashboard sharing settings modal and view a module that has been shared.
  • The easiest one is enabling/disabling the Idea Hub feature flag.
  • Ensure the Idea Hub feature flag is enabled.
  • Make a change to a module and verify that it works.
  • Disable the Idea Hub feature flag.
  • Make a change to a module and verify that it doesn't throw any API error or display any error notices.

Changelog entry

  • Prevent error from appearing when sharing data exists for a non-existent module.
@felixarntz felixarntz added Type: Bug Something isn't working P1 Medium priority labels Jun 30, 2022
@felixarntz felixarntz assigned felixarntz and unassigned felixarntz Jun 30, 2022
@nfmohit nfmohit assigned nfmohit and unassigned nfmohit Jun 30, 2022
@aaemnnosttv aaemnnosttv self-assigned this Jul 1, 2022
@aaemnnosttv
Copy link
Collaborator

Thanks @nfmohit, your suggested approach would work, but let's adjust it slightly.

  • Create a new public method named does_module_exist() which accepts the parameter $slug

Let's use module_exists(slug) instead which is a bit more consistent with what we have and core convention (e.g. metadata_exists, term_exists)

  • Inside the method, just return a boolean value checking if the provided $slug exists in get_available_modules().

Here we can simply use get_module(slug) in a try/catch returning true if it doesn't throw, and false if it does.

  • Inside this array_walk callback, before getting the module data, do a check if the module exists using the new does_module_exist() method.

Note we need the same check in the callback for add_option_ and update_option_ there, not just the one place.

@aaemnnosttv aaemnnosttv assigned nfmohit and unassigned aaemnnosttv Jul 1, 2022
@nfmohit
Copy link
Collaborator

nfmohit commented Jul 2, 2022

Thank you for the kind and detailed review @aaemnnosttv! I've updated the IB according to your suggestions.

@nfmohit nfmohit assigned aaemnnosttv and unassigned nfmohit Jul 2, 2022
@aaemnnosttv
Copy link
Collaborator

Great, thanks!

IB ✅

@hussain-t
Copy link
Collaborator

@makiost, could you update the QAB with more information?

View a module that has been shared, but that has been removed. (Easies one is the idea hub feature flag in settings)
Ensure that no critical errors display in the module.

For example,

  • Open the dashboard sharing settings modal and view a module that has been shared.
  • The easiest one is enabling/disabling the Idea Hub feature flag.
  • Ensure the Idea Hub feature flag is enabled.
  • Make a change to a module and verify that it works.
  • Disable the Idea Hub feature flag.
  • Make a change to a module and verify that it doesn't throw any API error or display any error notices.

@maciejost maciejost assigned hussain-t and unassigned maciejost Jul 7, 2022
@hussain-t hussain-t removed their assignment Jul 7, 2022
@tofumatt tofumatt assigned tofumatt and maciejost and unassigned tofumatt Jul 12, 2022
@maciejost maciejost removed their assignment Jul 14, 2022
tofumatt added a commit that referenced this issue Jul 14, 2022
…existing-modules

Prevent fatal error from sharing data being available for a non-existent module.
@tofumatt tofumatt removed their assignment Jul 14, 2022
@mohitwp mohitwp self-assigned this Jul 15, 2022
@mohitwp
Copy link
Collaborator

mohitwp commented Jul 15, 2022

QA Update ✅

Verified :

  • Tested this on develop branch.
  • Verified this for IDEA hub feature flag by following QAB. Now we are not getting API error.
  • Also verified for other module like PSI and analytics via disconnecting module. It is not creating any API error.

cc @wpdarren

@mohitwp mohitwp removed their assignment Jul 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 Medium priority PHP Type: Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

7 participants