Active
Project:
Drupal core
Version:
main
Component:
other
Priority:
Normal
Category:
Plan
Assigned:
Unassigned
Reporter:
Created:
29 Nov 2017 at 18:26 UTC
Updated:
29 Nov 2017 at 21:10 UTC
Jump to comment: Most recent
Problem/Motivation
Our BC and deprecation policies do not clarify how to handle the case where we need to move a method down a non-internal interface hierarchy. Since we are not allowed to remove the methods from the parent interface A, theoretically we need to deprecate them. However if the method is redefined by a descendant interface B, IDEs may treat its implementations as deprecated (PHP Storm does) as the topmost method definitions are deprecated.
An example of this situation is RevisionableStorageInterface, whose aim is to collect all methods providing revision support for entity storage classes. EntityStorageInterface already provides two revision-related methods and all non-content storage classes are forced to provide NULL implementations for those.
Ideally we would remove these methods and none would notice, however this is theoretically a BC break because if you have code relying on EntityStorageInterface::loadRevision() and an implementation of the "updated" EntityStorageInterface not defining it is passed to it, your code breaks.
Proposed resolution
In #2926540: Split revision-handling methods to a separate entity storage interface we decided to use @todo instead of @deprecated tags but keep the deprecation format in the PHP docs. We also created a follow-up to actually deprecate those methods before the last Drupal 8 minor is released. This would minimize the hassle provided by IDEs complaining that perfectly legal methods are deprecated. The follow-up is mentioned in the PHP docs as well, via a @see tag.
A possibility to avoid the deprecation in any 8.x.x branch and still warn contrib module authors preparing for D9, could be to add trigger_error() calls to the NULL implementations. This way (theoretical) API consumers relying on the deprecated definitions would actually be warned and could adapt and switch to a different implementation.
Remaining tasks
- Propose a valid solution
- Obtain the required signoffs
- Update the BC and deprecation policies
User interface changes
None
API changes
None.
API changes could possibly be a consequence of the policy change.
Data model changes
None
Comments
Comment #2
plachMinor celan-up
Comment #3
xjm@catch suggested another way of accomplishing this would be to create an entirely new replacement for EntityStorageInterface that doesn't implement the new revisioning interface, make EntityStorageInterface extend the new interface, make the base class implement the new interface instead of the old one, and then deprecate the old one.
Comment #4
catchSo something like:
Drupal\Core\Entity\Storage\EntityStorageInterface which is clean.
Update everywhere that expects Drupal\Core\Entity\EntityStorageInterface to use the new one.
This leaves the base class - if we just update it to implement the new interface, what happens to contrib code that is type-hinting the old interface? If that's a real problem, we might need a new 'clean' base class too, deprecate the current one (extending from the new base class + manually implementing the deprecated interface).