Problem/Motivation
\Drupal\layout_builder\Entity\LayoutEntityDisplayInterface::isOverridable()
\Drupal\layout_builder\Entity\LayoutEntityDisplayInterface::setOverridable()
\Drupal\layout_builder\DefaultsSectionStorageInterface::isOverridable()
\Drupal\layout_builder\DefaultsSectionStorageInterface::setOverridable()Proposed resolution
Make a shared OverridableInterface
Remaining tasks
User interface changes
API changes
Data model changes
Issue fork drupal-2985362
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 2985362-create-an-interface
changes, plain diff MR !6429
Comments
Comment #4
tim.plunkettComment #15
danielvezaOpened a MR for this change
Comment #16
mstrelan commentedThis is straight forward and does as described. Only thing I'd say is we can just call it
OverridableInterfaceinstead ofLayoutBuilderOverridableInterfaceas per the proposed resolution, but this is consistent withLayoutBuilderEnabledInterfaceso I'm not going to hold it up.I also wondered if we need a change record, but if you're implementing one of the existing interfaces you'll automatically be implementing the new interface already, so you don't need to declare the implementation in your class.
Comment #17
danielvezaYup - I had the same thoughts while developing this. I figured keeping it consistent with LayoutBuilderEnabledInterface was the right call, but I don't feel strongly either way
Good question. I guess it could be worth it since it's a new interface, but I'm not sure if it's one that will be used on it's own very often. I'm happy to write up a quick one if needed
Comment #18
quietone commentedI'm triaging RTBC issues. I read the IS, the comments ant the MR. I didn't find any unanswered questions.
Regarding the name, there are no existing interfaces in core using the word 'overridable' plus 'overridable' is not in the American English dictionaries recommended by Drupal.org, and it is in the list of misspelled words used by cspell. I think it we should conform to current patterns and change it.
There are existing interfaces in core using the word 'override';
Will
LayoutBuilderOverrideInterfaceconvey the same meaning?Setting to NW for naming. At least, we should not be using, 'overridable'
Comment #19
mstrelan commentedIn that case we need to rename isOverridable method to canBeOverridden. I'd have to disagree with this. We have things like Stringable, I think this is fine as is.
Comment #20
acbramley commentedThe word already has precedent in the method name
isOverridableso I'm not sure how it could be against cspell? It definitely is a word in the english language, do we need to expand the dictionary?Changing the semantics of this and having to do BC for changing the method name seems like unnecessary busy work.
Comment #21
kim.pepperI agree with #19 and #20. There is a strong precedent in Symfony and the PHP language itself of appending the "-able" suffix to an interface name. I don't think it needs to be in the dictionary.
Comment #22
longwaveAgree that renaming the existing methods is out of scope here, we just want to add a shared interface to the hierarchy.
Committed and pushed a99e0b5763 to 11.x and 277cf4a8bb to 10.3.x. Thanks!