Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
A Plugin without an id is not a useful plugin ===> let's throw an exception for it.
Comment | File | Size | Author |
---|---|---|---|
#23 | interdiff.1868916.20-23.txt | 756 bytes | longwave |
#23 | 1868916-23.drupal.Pluginphp-should-throw-an-exception-for-missing-id.patch | 4.01 KB | longwave |
Comments
Comment #1
dawehner.
Comment #2
dawehnerThis would a good improvement for DX, as it removes a potential point of failure.
Comment #3
XanoWe shouldn't just add this to the annotation class, as annotated class discovery is just one of the many available discovery mechanisms (YAML and static discovery are used as well). #2458789: Introduce PluginDefinitionInterface would be an ideal place for this, but as that's not in yet and plugin managers already contain functionality to merge in default values, maybe we should add the validation there.
Comment #4
dawehnerWell, the advantage of putting it onto the annotation class would be that you could throw a really helpful message to the endeveloper,
unless on the manager level where you have a level of abstraction. What about doing both?
Comment #5
XanoWe essentially provide two annotation base classes:
Plugin
, which supports annotation arrays and which this patch adds validation to, andAnnotationBase
, which is used by child classes with both single and multiple values. The latter is a little more difficult to change because of that.Comment #12
jhedstromThe patch in #5makes a ton of sense to me. Anything left to be done here?
Comment #13
jhedstromThose 2 failing tests need to be resolved. Looks like it's just a matter of updating some parameters to the exception that is thrown.
Comment #15
XanoRe-testing on 8.7.x.
Comment #16
XanoComment #17
XanoComment #18
XanoRight....
Comment #19
longwaveThis comment is no longer true! This assertion seems a bit pointless now, maybe replace it with one that checks the exception?
Comment #20
XanoGood one! What about this?
Comment #21
longwaveLooks good.
Comment #22
alexpottWe still need to test on PHP 5.6 which for component testing means doing something like
There's no helper here because we're using the PHPUnit TestCase class.
Comment #23
longwaveFixed #22
Comment #24
XanoAh, our components :) Thanks for picking up the fix, @longwave!
Comment #25
XanoI added a change record in case anyone runs into this.
Comment #26
alexpottThinking this through a bit more. I think we should trigger a deprecation here and tell people in Drupal 9 we're going to through an exception. That means we don't break any Drupal 8 code and people are warned and then in Drupal 9 we change the @trigger_error() to thrown an exception. It is possible that some contrib/custom tests are doing the same thing as core tests and there's no need to break them in Drupal 8 without warning.
Comment #27
XanoThat's a fair point. Conceptually, were IDs ever meant to be required? If so, allowing tests to omit IDs continues being a less-than-ideal example, though. What if we move the validation code to a protected method? Child annotations can then override and alter it, allowing IDs to be omitted, for instance, while allowing the base class to behave more like the way we'd like it to. This would also allow annotations to make IDs optional and generate IDs on the fly, for instance. Basically, as long as
Plugin
outputs an ID, we don't mind where it comes from.