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: cleanup constants and refactor autoload handling #278

Closed
wants to merge 0 commits into from

Conversation

justlevine
Copy link
Contributor

What

This PR fixes the plugin's naive autoload detection which previously would:

  1. throw a fatal error if dependencies were not installed (instead of the intended admin_notice)
  2. show an admin_notice if the plugin was installed as a composer dependency itself (even though the plugin dependencies would get installed).

How

  1. Autoload handling was moved to \WPGraphQL\ContentBlocks\Autoloader, where it checks (in order):
    a. if WPGRAPHQL_CONTENT_BLOCKS_AUTOLOAD wants us to skip autoloading
    b. if we've previously loaded the dependencies (via a static property)
    c. if the WPGraphQLContentBlocks class exists (that means it's being autoloaded already)
    d. and only then if there is a local vendor/autoload.php file.
  2. The autoloader now runs in wp-graphql-content-blocks.php::wpgraphql_content_blocks_init(), where it is required_onceed instead of the (now autoloaded) WPGraphQLContentBlocks class.
  3. To support this, the several constants that were defined in WPGraphQLContentBlocks have been moved to the pluggable wp-graphql-content-blocks.php:wpgraphql_content_blocks_constants(), (The versioning script was updated to accomodate the change)
  4. Since the constants were already being merged to a single source, I took a moment to dedupe and deprecate the old ones:
  • WPGRAPHQL_CONTENT_BLOCKS_FILE => WPGRAPHQL_CONTENT_BLOCKS_PLUGIN_FILE
  • WPGRAPHQL_CONTENT_BLOCKS_PATH => WPGRAPHQL_CONTENT_BLOCKS_PLUGIN_PATH
  • WPGRAPHQL_CONTENT_BLOCKS_MIN_PHP_VERSION => no replacement (this seems to be an artifact from wp-graphql core boilerplate).

To test

  1. Create a composer-based WordPress install (e.g. bedrock)
  2. Add the branch to the composer.json repositories:
"repositories": [
  {
    "type": "vcs",
    "url": "https://github.com/rtcamp/wp-graphql-content-blocks.git"
  },
],
  1. Install the plugins as composer dependencies
  composer require wpackagist-plugin/wp-graphql wpengine/wp-graphql-content-blocks:dev-fix/constants-autoloading
  1. Go to the Plugins Page, activate the plugin, see everything works with no missing deps admin notice.

Additional notes

@todo tags are used for version numbers and should be replaced once we know what the version will be.

@justlevine justlevine requested a review from a team as a code owner September 11, 2024 21:37
Copy link

changeset-bot bot commented Sep 11, 2024

⚠️ No Changeset found

Latest commit: 766737d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@jasonbahl
Copy link
Contributor

@justlevine on quick scan I see the new AutoLoader.php but I don't see a change that invokes the class in in the main plugin?

@justlevine
Copy link
Contributor Author

fml i didnt push everything 1 sec

@justlevine on quick scan I see the new AutoLoader.php but I don't see a change that invokes the class in in the main plugin?

@justlevine justlevine force-pushed the fix/constants-autoloading branch from b2516bf to 1d4fdc9 Compare September 11, 2024 21:46
@jasonbahl
Copy link
Contributor

fml i didnt push everything 1 sec

Amazing that I caught you with a case of my famous "Moving too fasts". Usually it's me and not you that's suffering from this! 😆

@justlevine
Copy link
Contributor Author

justlevine commented Sep 11, 2024

@jasonbahl force-pushed with all the files this time i promise 🤣

@@ -68,7 +68,7 @@ async function bumpStableTag(readmeTxt, version) {
async function bumpVersionConstant(pluginFile, version) {
return bumpVersion(
pluginFile,
/^\s*\$this->define\(\s*'WPGRAPHQL_CONTENT_BLOCKS_VERSION', '([0-9.]+)/gm,
/^\s*define\(\s*'WPGRAPHQL_CONTENT_BLOCKS_VERSION', '([0-9.]+)/gm,
Copy link
Contributor

Choose a reason for hiding this comment

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

while we're making changes to the versionPlugin.js. . .we might be able to satisfy the replacing of @since @todo references as well 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Not required to approve this PR, just a thought though. If we standardize on @since @todo or @since @next or something like that, then we could automate the update with the rest of the changesets version updates 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

regular @todo like used in the _doing_it_wrong( '...', '@todo' ) functions might be a bit broad. . .although we could regex replace that like we do the WPGRAPHQL_CONTENT_BLOCKS_VERSION above? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's outta my wheelhouse, I had to triple check this "simple" change 🙃

Copy link
Contributor

Choose a reason for hiding this comment

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

haha! all good. I can follow-up on that separately. . .would just need to make a standard of how we add the placeholders.

like is it always:

_doing_it_wrong( 'some string', '@todo'  );

and

/**
 * Some docblock
 * @since @todo
 */

etc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean that's how I do it, but really out of a general habit of checking all @todos before pushing a release.

To make it easier to regex regardless of if its _doing_it_wrong(), _deprecated_*(), @since or even a regular doc-block reference, maybe we intentionally want to use/reserve @next or @next-version? then we can just match the annotation instead of the wrapper

Copy link
Contributor

Choose a reason for hiding this comment

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

ya, was thinking @next-version as well

Copy link
Contributor

Choose a reason for hiding this comment

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

that way @todo can be left for other uses of @todo that IDEs already have tooling to pick up, etc 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So long as it doesnt make phpcs complain

@justlevine justlevine force-pushed the fix/constants-autoloading branch from 1d4fdc9 to 248f68e Compare September 16, 2024 19:48
@justlevine justlevine closed this Sep 19, 2024
@justlevine justlevine force-pushed the fix/constants-autoloading branch from 248f68e to 766737d Compare September 19, 2024 10:22
@justlevine
Copy link
Contributor Author

This was squashed in #279

@justlevine justlevine deleted the fix/constants-autoloading branch September 19, 2024 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants