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

Seperate core and contrib api files #774

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

bbrala
Copy link

@bbrala bbrala commented Jun 19, 2024

Fixes #770

  1. Added new options to bleeding edge. Current behaviour is the same.
  2. Trigger deprecation notice when using old parameter, did not add a version since i dont know when it will be removed
  3. Added defaults as per related issue. Behaviour is the same, but can be specified.
  4. Wanted to add a test for a core module, but not sure how so i didnt.

Copy link
Contributor

@Kingdutch Kingdutch left a comment

Choose a reason for hiding this comment

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

This looks good to me (I'm still a proponent of immediately setting checkCoreDeprecatedHooksInApiFiles to true but I know Matt wants to do it in a separate minor release).

Commenting rather than explicitly approving because I don't know if we can already remove checkCoreDeprecatedHooksInApiFiles anywhere. For example from bleedingEdge.neon, in order to prevent people from getting deprecation notices for something they load from the package itself.

Not entirely sure what the difference in behaviour between bleedingEdge.neon and extension.neon is though (since the values also differ in those two files).

bleedingEdge.neon Outdated Show resolved Hide resolved
extension.neon Show resolved Hide resolved
Comment on lines +131 to +134
// Trigger deprecation error if checkDeprecatedHooksInApiFiles is enabled.
if ($drupalParams['bleedingEdge']['checkDeprecatedHooksInApiFiles']) {
trigger_error('The bleedingEdge.checkDeprecatedHooksInApiFiles parameter is deprecated and will be removed in a future release.', E_USER_DEPRECATED);
}
Copy link
Owner

Choose a reason for hiding this comment

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

ah this is how

Copy link
Author

Choose a reason for hiding this comment

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

Just missing version, but that's OK I think?

Copy link
Author

Choose a reason for hiding this comment

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

I've updated bleedingedge so when people copy that they won't get a deprecation notice.

Copy link
Author

Choose a reason for hiding this comment

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

Might need an isset or something when we remove the setting? Or perhaps even now.

@bbrala
Copy link
Author

bbrala commented Jun 20, 2024

This looks good to me (I'm still a proponent of immediately setting checkCoreDeprecatedHooksInApiFiles to true but I know Matt wants to do it in a separate minor release).

Commenting rather than explicitly approving because I don't know if we can already remove checkCoreDeprecatedHooksInApiFiles anywhere. For example from bleedingEdge.neon, in order to prevent people from getting deprecation notices for something they load from the package itself.

Not entirely sure what the difference in behaviour between bleedingEdge.neon and extension.neon is though (since the values also differ in those two files).

Good point about bleeding edge. Updated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants