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

VIP Parse.ly: Check for Support User class exists in the vip-parsely plugin #6016

Merged
merged 4 commits into from
Dec 4, 2024

Conversation

vaurdan
Copy link
Member

@vaurdan vaurdan commented Nov 26, 2024

Description

This PR adds a new defensive check in the VIP Parse.ly plugin, that prevents the VIP-specific customizations of the WP Parse.ly plugin to load when WP_INSTALLING is set and true.

This fixes a fatal error being thrown when accessing the example.com/wp-activate.php URL. Since the vip-support plugin is not loaded when WP_INSTALLING is defined, this extra validation short circuits the wp_parsely_current_user_can_use_pch_feature callback to return the original value, if the Automattic\VIP\Support_User\User class does not exists.

PHP Fatal error:  Uncaught Error: Class "Automattic\VIP\Support_User\User" not found in /var/www/wp-content/mu-plugins/vip-parsely/vip-parsely.php:93
Stack trace:
#0 /var/www/wp-includes/class-wp-hook.php(312): {closure}(NULL, 'smart_linking', Object(WP_User))
#1 /var/www/wp-includes/plugin.php(205): WP_Hook->apply_filters(NULL, Array)
(...)

Changelog Description

Fixed

  • VIP Parse.ly: Fixed a fatal error that happened in certain conditions within the VIP Parse.ly plugin.

Pre-review checklist

Please make sure the items below have been covered before requesting a review:

  • This change works and has been tested locally or in Codespaces (or has an appropriate fallback).
  • This change works and has been tested on a sandbox.
  • This change has relevant unit tests (if applicable).
  • This change uses a rollout method to ease with deployment (if applicable - especially for large scale actions that require writes).
  • This change has relevant documentation additions / updates (if applicable).
  • I've created a changelog description that aligns with the provided examples.

Pre-deploy checklist

  • VIP staff: Ensure any alerts added/updated conform to internal standards (see internal documentation).

@vaurdan vaurdan self-assigned this Nov 26, 2024
@vaurdan vaurdan requested a review from a team as a code owner November 26, 2024 09:31
Copy link

codecov bot commented Nov 26, 2024

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 30.53%. Comparing base (00c35f2) to head (133744d).
Report is 4 commits behind head on develop.

Files with missing lines Patch % Lines
vip-parsely/vip-parsely.php 0.00% 2 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             develop    #6016      +/-   ##
=============================================
+ Coverage      30.52%   30.53%   +0.01%     
  Complexity      4811     4811              
=============================================
  Files            289      289              
  Lines          21175    21165      -10     
=============================================
  Hits            6463     6463              
+ Misses         14712    14702      -10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mjangda
Copy link
Member

mjangda commented Nov 28, 2024

This still isn't quite the best fix and likely bound to cause other problems in the future. It suggests that someone (customer code or our code) is doing something in a context they shouldn't.

If our short-term goal is to just stop the fatals from happening, I would just do a class_exists check inside the callback for wp_parsely_current_user_can_use_pch_feature before we use the Automattic\VIP\Support_User\User class. (It's a bit of anti-pattern since most of mu-plugins code runs on the assumption that key VIP code like the Support User functionality is correctly loaded inside late hooks / filters).

@vaurdan
Copy link
Member Author

vaurdan commented Nov 29, 2024

Thank you for the feedback @mjangda!

It suggests that someone (customer code or our code) is doing something in a context they shouldn't.

I don't think that is anything wrong per-se in the customer code that triggers this issue: there is an API preload that runs on wp_footer and uses rest_preload_api_request. This API preloading consequently initializes the REST API (via rest_api_init), which then also initializes the Parse.ly REST API (and all its dependencies).

Since wp-activate.php calls the wp_footer action, it will also run on this context, loading the REST API, then Parse.ly, and then the callback - wp_parsely_current_user_can_use_pch_feature - that triggers the fatal.

I think part of the problem is having the VIP Support plugin not loading on this specific scenario (WP_INSTALLING defined and true). I'm sure there's a reason for that, however a safer alternative would be to validate if the constant is not true only in the necessary blocks of the code that shouldn't run during WP installs/upgrades.

It's a bit of anti-pattern since most of mu-plugins code runs on the assumption that key VIP code like the Support User functionality is correctly loaded inside late hooks / filters

Yes, that's partly the reason why I tried to keep that logic outside the wp_parsely_current_user_can_use_pch_feature callback. But I do see why you saying that it might cause problems in the future - right now, nothing on the Parse.ly plugin needs to run when WP_INSTALLING is set, however if that changes this could cause unexpected issues.

I'm going to change it to check for the class inside the callback as suggested, so we can stop the fatals that are currently happening.

@vaurdan vaurdan requested review from sjinks and mjangda November 29, 2024 12:25
@vaurdan vaurdan changed the title VIP Parse.ly: Check for WP_INSTALLING in the vip-parsely plugin VIP Parse.ly: Check for Support User class exists in the vip-parsely plugin Nov 29, 2024
@sjinks sjinks requested a review from Copilot December 4, 2024 13:50

Choose a reason for hiding this comment

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

Copilot wasn't able to review any files in this pull request.

Files not reviewed (1)
  • vip-parsely/vip-parsely.php: Language not supported
@acicovic
Copy link
Contributor

acicovic commented Dec 4, 2024

@sjinks, the Copilot review doesn't seem to have worked. Are we OK to merge this? If yes, feel free to do so.

Copy link

sonarqubecloud bot commented Dec 4, 2024

@rinatkhaziev rinatkhaziev merged commit 97c10f8 into develop Dec 4, 2024
34 checks passed
@rinatkhaziev rinatkhaziev deleted the fix/vip-parsely-wp-activate-fatal branch December 4, 2024 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants