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

[TECTRIA-668] PUE Checker Transient Init #2354

Open
wants to merge 13 commits into
base: release/T25.antman
Choose a base branch
from

Conversation

redscar
Copy link
Contributor

@redscar redscar commented Jan 20, 2025

🎫 Ticket

TECTRIA-668

🗒️ Description

This PR addresses a logic issue in the license checker. When a plugin is initially activated, it does not automatically check the license status. This results in issues where licenses are not recognized as valid because the required transient and option values are not set. As a result, the system incorrectly assumes no valid licenses are active, even when they should be.

To resolve this, I've added the initialize_license_check logic, which:

Runs on the init hook.
Validates whether the license transient and option exist.
If missing, triggers a license check to ensure licenses are properly validated.

Additionally:

All deprecated code in this file has been removed.
Docblocks have been updated for clarity and accuracy.
Type hints have been added to methods for improved code reliability and maintainability.

Testing Notes:

All original tests pass successfully with these changes.
The new logic has been tested to ensure it correctly handles license checks when no transient or option is set.

🎥 Artifacts

✔️ Checklist

  • Ran npm run changelog to add changelog file(s). More info here
  • Code is covered by NEW wpunit or integration tests.
  • Code is covered by EXISTING wpunit or integration tests.
  • Are all the required tests passing?
  • Automated code review comments are addressed.
  • Have you added Artifacts?
  • Check the base branch for your PR.
  • Add your PR to the project board for the release.

@redscar redscar added the hold Status: on hold–do not proceed with other status items. label Jan 20, 2025
@redscar redscar self-assigned this Jan 20, 2025
@redscar redscar removed the hold Status: on hold–do not proceed with other status items. label Jan 24, 2025
@redscar redscar marked this pull request as ready for review January 24, 2025 16:39
@redscar redscar added the code review Status: requires a code review. label Jan 24, 2025
Copy link
Member

@bordoni bordoni left a comment

Choose a reason for hiding this comment

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

There are more instances of the behavior I made two comments around a enforced typing that could break due to filters. Please take a look at the code to make sure we dont hit those.

src/Tribe/PUE/Checker.php Outdated Show resolved Hide resolved
src/Tribe/PUE/Checker.php Outdated Show resolved Hide resolved
src/Tribe/PUE/Checker.php Outdated Show resolved Hide resolved
src/Tribe/PUE/Checker.php Outdated Show resolved Hide resolved
src/Tribe/PUE/Checker.php Outdated Show resolved Hide resolved
src/Tribe/PUE/Checker.php Outdated Show resolved Hide resolved
Copy link
Member

@Camwyn Camwyn left a comment

Choose a reason for hiding this comment

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

A lot of docblock fixes

src/Tribe/PUE/Checker.php Outdated Show resolved Hide resolved
src/Tribe/PUE/Checker.php Outdated Show resolved Hide resolved
src/Tribe/PUE/Checker.php Outdated Show resolved Hide resolved
src/Tribe/PUE/Checker.php Outdated Show resolved Hide resolved
src/Tribe/PUE/Checker.php Outdated Show resolved Hide resolved
src/Tribe/PUE/Checker.php Outdated Show resolved Hide resolved
src/Tribe/PUE/Checker.php Outdated Show resolved Hide resolved
src/Tribe/PUE/Checker.php Outdated Show resolved Hide resolved
src/Tribe/PUE/Checker.php Outdated Show resolved Hide resolved
*
* @since TBD
*
* @param array $query_args The query arguments to be sent to the API.
Copy link
Member

Choose a reason for hiding this comment

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

No...
$callback?

Copy link
Member

Choose a reason for hiding this comment

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

Hm, is this hook working as expected then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Camwyn That's a good point. I actually don't see this function being used at all anywhere.

Looks like instead of calling the function in other spots we are using just the filter.

* @param array|object $args Arguments used to query for installer pages from the Plugin Install API.
*
* @return mixed
*/
public function inject_info( $result, $action = null, $args = null ) {
public function inject_info( $result, string $action = null, $args = null ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just saw this... Should we not make the second param nullable given it can be either?

Suggested change
public function inject_info( $result, string $action = null, $args = null ) {
public function inject_info( $result, ?string $action = null, $args = null ) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code review Status: requires a code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants