Closed (fixed)
Project:
Drupal core
Version:
11.x-dev
Component:
entity system
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
25 Dec 2024 at 08:21 UTC
Updated:
11 Mar 2025 at 22:19 UTC
Jump to comment: Most recent
Comments
Comment #2
nikolay shapovalov commentedLooks like need to split this to several issues.
Comment #4
nikolay shapovalov commentedComment #5
nikolay shapovalov commentedComment #6
nikolay shapovalov commentedComment #7
nikolay shapovalov commentedComment #8
nikolay shapovalov commentedForget about entity_test_delete_bundle(). Update IS. Double checked, all methods from module are covered.
Comment #9
berdirReviewed a bit, I'd suggest to postpone this on the two child issues, we won't be able to remove the .module file due to the BC layer anyway though.
Comment #10
nikolay shapovalov commentedMR rebased
Comment #11
nicxvan commentedThe children are in this can be updated.
We can't remove the deprecated but we can make it easier in the future.
Comment #12
nikolay shapovalov commentedMR updated
Comment #13
berdirVerified that there's nothing left in entity_test.module except the two deprecated functions.
I'm not sold yet on the Callbacks class/pattern used here in general, but for this module, it's an existing pattern that's just used here too.
Comment #15
catchYeah I don't love the callbacks either but also can't get overly concerned about them considering this is test modules.
Comment #17
joelpittetA long shot but could these helpers be committed on D10 or how do we use them in contrib to support D10? I tried copying search_api in #3505316: Fix reliance on deprecated .MODULE.inc files for hooks inside og but found D10 didn't have them https://git.drupalcode.org/project/og/-/jobs/4465178
Comment #18
nicxvan commentedDeprecationHelper is the standard way https://www.drupal.org/node/3379306
Comment #19
joelpittet@nicxvan What should I do when the class doesn't exist, yet like in this case in my test for this change #3495966: Deprecate and replace entity_test_create_bundle(), entity_test_delete_bundle()?
Comment #20
berdir@joelpittet: A function/method exists check for either the old or the new one, combined with a phpstan-ignore-next-line for the BC path.
FWIW, this is exactly why I think having phpstan set to not allow fail is a a problem, especially on next minor. This is a deprecation, an early warning that this will stop working in ~2 years. There is absolutely no reason to break CI now and disrupt contribution workflows. You can disable phpstan on next major or allow it to fail. See https://git.drupalcode.org/project/diff/-/blob/2.x/.gitlab-ci.yml?ref_ty....
If phpstan is used purely as a code quality tool it's one thing, and it's also different in custom projects where you only care about one specific core version, but considering that we also use phpstan as this early-warning-system for deprecations, it is IMHO just not a good fit. Maybe there could be separate jobs that filter things depending on whether or not it's a deprecation message. But that's getting way off topic.
Comment #21
joelpittet@berdir, Thanks for your insight! I ended up just copying the helper method into the test class that was using it, bypassing both the original helper function and the missing helper class altogether. It might come back to bite me later, but that’s a future problem. 😉 If the phpstan on next major causes too much of this, I will do as you say and allow fails, though gotta feel the pain first.
Comment #22
berdirYour decision, but I think that's the wrong call. Those helpers are only one side of the coin. The other side is \Drupal\entity_test\Hook\EntityTestHooks::entityBundleInfo(). If you only copy one side, then if the implementation changes then it will break. Those two are tightly coupled.
My suggestion isn't that complicated, it's essentially something like this: