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: prevent conflicts with old plugin #67

Merged
merged 13 commits into from
Jul 27, 2023

Conversation

jasonbahl
Copy link
Contributor

@jasonbahl jasonbahl commented Jul 24, 2023

What does this implement/fix? Explain your changes.

This ensures actions and filters are unique to this version of the plugin. This means that any custom code targeting hooks and filters of the previous WPGraphQL for ACF will not execute when this plugin is active and won't cause unintended side effects.

This also adjusts Class names to be unique to ensure any code from extending codebases don't accidentally all code from this version of the plugin.

What currently open issues does this close or contribute?

closes #65
closes #66

Other Notes

Failing Schema Linter

Note, the Schema Linter test is failing because the previous release action did not upload the Schema Artifact to the release: https://github.com/wp-graphql/wpgraphql-acf/releases/tag/v2.0.0-beta.3.1.0

Action / Filter comparison across versions

2.0.0-beta.4.0 converts to a wpgraphql/acf/${hook_name} convention.

We believe this is a bit more readable and makes it more clear what the filter/hook is intended for.

We also believe this will help with folks upgrading. If anyone had custom code that extended v0.6 or prior, it might be easy to miss that it's not working because the hook name is graphql_acf_* instead of wpgraphql_acf_* where it might be easily overlooked that the wp was added, thus causing much time banging heads against a desk and pulling hair out before realizing the change.

We believe it will be more clear if the user is looking at docs for the latest version of WPGraphQL and sees wpgraphql/acf/* they would know immediately that their old action with graphql_acf_* would not be firing, thus saving hair and desks.

v0.6.3 v2.0.0-beta.3.1.0 v2.0.0-beta.4.0
wpgraphql_acf_should_field_group_show_in_graphql graphql_acf_should_field_group_show_in_graphql wpgraphql/acf/should_field_group_show_in_graphql
graphql_acf_get_root_id n/a wpgraphql/acf/pre_get_node_acf_id
graphql_acf_field_value graphql_acf_field_value wpgraphql/acf/field_value
wpgraphql_acf_supported_fields wpgraphql_acf_supported_field_types wpgraphql/acf/supported_field_types
wpgraphql_acf_register_graphql_field n/a n/a
graphql_acf_post_object_source n/a n/a
n/a n/a wpgraphql/acf/get_graphql_field_config
n/a graphql_acf_is_acfe_active wpgraphql/acf/is_acfe_active
n/a graphql_acf_field_type_default_admin_settings wpgraphql/acf/field_type_default_admin_settings
n/a graphql_acf_field_type_admin_settings wpgraphql/acf/field_type_admin_settings
n/a graphql_acf_excluded_admin_field_settings wpgraphql/acf/excluded_admin_field_settings
n/a graphql_acf_pre_resolve_acf_field wpgraphql/acf/pre_resolve_acf_field
n/a graphql_acf_get_registered_field_types wpgraphql/acf/get_registered_field_types
graphql_acf_init graphql_acf_init wpgraphql/acf/init
graphql_acf_match_location_rule graphql_acf_match_location_rule wpgraphql/acf/match_location_rule
n/a graphql_acf_register_field_types wpgraphql/acf/register_field_types
n/a graphql_acf_registry_init wpgraphql/acf/registry_init

- update phpcs.xml.dist rules to better align with WPGraphQL core
… from accidentally adding classes that match the old version of the plugin. We don't want old functions to be available in the new plugin, intentionally.
- update testing-integration workflow to test against WP 6.2
@coveralls
Copy link

coveralls commented Jul 24, 2023

Pull Request Test Coverage Report for Build 848aaa5ef215a8bc8f4ec974fd57083aee8d9928-PR-67

  • 10 of 20 (50.0%) changed or added relevant lines in 7 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.01%) to 56.78%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/LocationRules/LocationRules.php 0 1 0.0%
src/Utils.php 4 5 80.0%
src/WPGraphQLAcf.php 0 2 0.0%
src/AcfGraphQLFieldType.php 0 3 0.0%
src/ThirdParty/AcfExtended/AcfExtended.php 0 3 0.0%
Files with Coverage Reduction New Missed Lines %
src/FieldConfig.php 1 79.49%
Totals Coverage Status
Change from base Build 6a9360506d51b13b7cc6c91b1ca9f547b96e0f0f: 0.01%
Covered Lines: 1499
Relevant Lines: 2640

💛 - Coveralls

@markkelnar
Copy link
Contributor

I see the action 'graphql_acf_match_location_rule'. Does that need to change?

@markkelnar
Copy link
Contributor

Is it intentional not to change the name of the actions that are triggered in Field Type Registry? graphql_acf_registry_init and graphql_acf_register_field_types.

@jasonbahl
Copy link
Contributor Author

Is it intentional not to change the name of the actions that are triggered in Field Type Registry? graphql_acf_registry_init and graphql_acf_register_field_types.

@markkelnar looks like I updated the filters but not the actions 🤦🏻‍♂️. Good catch. I updated the actions and the table documenting the differences.

@jasonbahl jasonbahl added the Compat: Breaking Change This is a breaking change to existing functionality label Jul 27, 2023
@jasonbahl jasonbahl merged commit 573fecb into main Jul 27, 2023
20 of 22 checks passed
@jasonbahl jasonbahl deleted the feat/prevent-conflicts-with-old-plugin branch January 24, 2024 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Compat: Breaking Change This is a breaking change to existing functionality
Projects
None yet
3 participants