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

Fix - TGM Plugin activation calling undefined function wc_get_screen_ids #44114

Merged
merged 5 commits into from Feb 17, 2024

Conversation

shivapoudel
Copy link
Contributor

@shivapoudel shivapoudel commented Jan 26, 2024

If we add woocommerce and enable the force_activation config in TGM plugin activation then the bug appears.

Submission Review Guidelines:

Changes proposed in this Pull Request:

image

How to test the changes in this Pull Request:

All this does is ensure that a particular file is loaded before a function within that file is called. Most of the time, the file is already loaded, so in those cases, this change does nothing. Rather than having to install and set up the third-party TGM Plugin, all we really need to do here is make sure this change doesn't break anything. So after switching to this branch, try a few things:

  • Deactivating and reactivating the plugin
  • Navigate to a few different WooCommerce pages within WP Admin

Doing these things, you shouldn't see any fatal errors about notices or about functions already being defined.

Changelog entry

  • Automatically create a changelog entry from the details below.

Significance

  • Patch
  • Minor
  • Major

Type

  • Fix - Fixes an existing bug
  • Add - Adds functionality
  • Update - Update existing functionality
  • Dev - Development related task
  • Tweak - A minor adjustment to the codebase
  • Performance - Address performance issues
  • Enhancement - Improvement to existing functionality

Message

Ensure WC admin functions are defined before attempting to add notices.

Comment

If we add woocommerce and enable the `force_activation` config in TGM plugin activation then the bug appears.
@github-actions github-actions bot added plugin: woocommerce Issues related to the WooCommerce Core plugin. type: community contribution labels Jan 26, 2024
@woocommercebot woocommercebot requested review from a team and coreymckrill and removed request for a team January 26, 2024 12:51
Copy link
Contributor

Hi ,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

Copy link
Collaborator

@coreymckrill coreymckrill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @shivapoudel, thanks for the PR! Rather than bailing early on the add_notices method when wc_get_screen_ids doesn't exist, I think it would be preferable to maintain the functionality and just ensure that wc_get_screen_ids does exist.

The reason the error is happening is that the wc-admin-functions.php file isn't getting loaded during the TGM request for some reason. But we could do something like what happens here, and require the file within add_notices. So maybe at the top of the method we could add a line like this:

require_once WC_ABSPATH . 'includes/admin/wc-admin-functions.php';

What do you think?

@coreymckrill coreymckrill added the needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. label Feb 15, 2024
@shivapoudel
Copy link
Contributor Author

shivapoudel commented Feb 15, 2024

But TGM is external library so how the inclusion occurs? Or you mean to say rather than function_exists check just to include the file within wc_get_screen_ids function.

Copy link
Collaborator

@coreymckrill coreymckrill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or you mean to say rather than function_exists check just to include the file within wc_get_screen_ids function.

Sort of. I mean something like the below suggestion.

@shivapoudel
Copy link
Contributor Author

@coreymckrill Adapted the changes. Thanks!

@coreymckrill
Copy link
Collaborator

Closing and re-opening this PR to fix the stuck CI actions...

@coreymckrill coreymckrill merged commit a14c59b into woocommerce:trunk Feb 17, 2024
41 of 42 checks passed
@github-actions github-actions bot added this to the 8.7.0 milestone Feb 17, 2024
@github-actions github-actions bot added the needs: analysis Indicates if the PR requires a PR testing scrub session. label Feb 17, 2024
@shivapoudel shivapoudel deleted the patch-1 branch February 20, 2024 05:00
@Stojdza Stojdza added needs: external testing Indicates if the PR requires further testing conducted by testers external to the development team. status: analysis complete Indicates if a PR has been analysed by Solaris and removed needs: analysis Indicates if the PR requires a PR testing scrub session. labels Feb 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. needs: external testing Indicates if the PR requires further testing conducted by testers external to the development team. plugin: woocommerce Issues related to the WooCommerce Core plugin. status: analysis complete Indicates if a PR has been analysed by Solaris type: community contribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants