Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
This is part of #1966246: [meta] Introduce specific annotations for each plugin type.
Now, when someone opens an entity type, the first thing they see is @EntityType, not @Plugin. Other than the namespace (which should be changeable), they don't need to know that entity types use plugin discovery.
And, now the EntityType class is the perfect place to document... itself.
Comment | File | Size | Author |
---|---|---|---|
#30 | entitytype-1967294-30.patch | 56.53 KB | tim.plunkett |
#30 | interdiff.txt | 4.69 KB | tim.plunkett |
#26 | interdiff.txt | 675 bytes | tim.plunkett |
#26 | entitytype-1967294-26.patch | 54.72 KB | tim.plunkett |
#23 | entitytype-1967294-23.patch | 54.72 KB | tim.plunkett |
Comments
Comment #1
tim.plunkettIt also lets us remove the need for @Translation, at least for known keys. Unknown and tacked-on keys can still use it.
Comment #2
msonnabaum CreditAttribution: msonnabaum commentedYes! This looks great.
Comment #3
tim.plunkettSo, instead of
we will have
Comment #4
Dave ReidDoes this hard-code what properties the annotation supports, and can no longer be extended by contrib?
Comment #5
tim.plunkettNope, it doesn't prevent contrib from doing anything.
It just hardcodes what it will automatically wrap in t(), so you don't have to use @Translate. Anything in contrib you add that you want wrapped in t(), you have to do @Translate("My string") yourself.
Comment #6
tim.plunkettIn fact, a good deal of these properties are tacked on by entity_translation, and shouldn't be on there.
We should have ET serve as the model for how one adds their own properties to an EntityType.
Comment #7
webchickWell this seems like an entirely sensible idea.
I'm never going to disagree with more explictness in what you mean, and I *hate* @Translation from the very pit of my soul. This also implies we could actually document what the hell properties an @EntityType has, which was one of quicksketch's main concerns when he first hit annotations.
Comment #8
dawehnerThe advantage of @Translation is that you can scan actually the list of available strings.
Comment #9
webchickWhy do you care, if magical fairy elves take care of it automatically for you? :)
Comment #10
webchickDamn. I knew it was too magical to be true.
Channeling Gábor from #1849610-36: Improve "add" links accessibility:
I don't think we can kill @Translation, sadly. :\ Though why it is @Translation and not @t is beyond me (and not for this issue).
Comment #11
tim.plunkettUm, well
How is that better?
Comment #12
Gábor Hojtsy#1933984: Support for Drupal 8 @Translation annotations is not yet solved in potx, but I feel it would be orders of magnitude easier to parse our strings wrapped in @Translation vs. trying to identify what kind of annotation this is and then what keys should be saved for translation from that kind of annotation (with static code analysis).
Comment #13
Dave ReidI would also point out it's a little bit of a DX-WTF if I only have to wrap certain strings in @Translation but others I shouldn't because it's taken care of for me. So me writing a new plugin, how do I know which should I use? I would rather we suffer and just have @Translation defined in each annotation rather than trying to make it automatic, and the job of potx harder.
Comment #14
tim.plunkettOkay, so well abandon that avenue for now. Still an improvement!
Comment #15
dawehnerJust wondering whether all controllers should be defined in the same place in this class.
Comment #16
tim.plunkettThis contains the patch in #1967420: Allow Core\AnnotatedClassDiscovery to pass all parameters to the constructor of Component\AnnotatedClassDiscovery
I copied all of the properties directly in the EntityManager, in the same order. Yes the order doesn't make sense yet. Yes some of the properties don't make sense :)
Comment #17
tim.plunkettOkay, work to do.
Comment #18
tim.plunkettOkay! This lets us kill the defaults processing too.
Comment #20
tim.plunkettWhoops. Forgot you can't do that:
Comment #21
xjmThis is so many flavors of awesome that we should make a sundae bar.
Comment #23
tim.plunkettOkay, showing off more of my inexperience with Reflection. This one should be good to go now.
Also, accidentally added a default for render_controller_class that didn't belong.
Comment #24
dawehnerAs discussed on IRC, we could also use get_object_vars($this) here, but well, that's just an option.
"\"
It's great that we don't need another discovery class, as this would be confusing for some people.
"\"
Comment #26
tim.plunkettUgh, I copypasted the default for $field_cache wrong, switching it back to TRUE as it is currently in HEAD.
Also, added the missing \ that @dawehner pointed out, thanks!
Talked to beejeebus about the Reflection vs get_object_vars(), he said not to change it unless we have documented performance reasons, since this is cleaner.
Comment #27
EclipseGc CreditAttribution: EclipseGc commentedThis looks like a HUGE improvement to my eyes. RTBC!
Eclipse
Comment #28
dawehnerJust in general I'm wondering whether/how we should document that a specific value is optional (for example render_controller_class).
Comment #29
webchickWhen we document parameters on hook_foo_info() and the like, we just prefix their description with "(optional) ".. we should probably do that here as well, so we don't lose that information.
Comment #30
tim.plunkettOkay, added in the (optional) information that was in EntityManager, and converted the brand spankin new Field and FieldInstance entities! Yay!
Comment #31
EclipseGc CreditAttribution: EclipseGc commentedIf it comes back green, I say RTBC.
Eclipse
Comment #32
msonnabaum CreditAttribution: msonnabaum commented♥♥♥
Comment #33
EclipseGc CreditAttribution: EclipseGc commentedComment #34
webchickF*ck yeah! This looks great!!
Committed and pushed to 8.x! :D Yay for a big step in the right direction of mere mortals being able to grok D8. :)
Comment #35
yched CreditAttribution: yched commentedMinor : unless I'm mistaken, If EntityManager has no more $defaults, it doesn't need ProcessDecorator() anymore ?
Comment #36
tim.plunkettYou're unfortunately mistaken, we still have all of this noise:
Now, half of our entity types don't need that, maybe we could move that into DatabaseStorageController where it belongs.
Comment #37
xjmDoesn't this need change notice updates?
Comment #38
tim.plunkettI've updated https://drupal.org/node/1827470, I think that's enough?
Comment #39
tim.plunkettOkay, and this one, we should be good now
https://drupal.org/node/1400186
Comment #40
tim.plunkettFollow-up: #1979152: Move translation_entity-specific entity type metadata out of the EntityType annotation