The getType() method is very generic and is sometimes in the way. We already introduced interfaces for all the basic data types and most uses got replaced.
This removes the method and updates the remaining calls. Either by an interfaceof check or by changing it to a getPluginId() call. I'm not yet sure if that's a valid replacement, need some feedback on that.
API changes
The TypedDataInterface::getType()
method is removed. Callers should instead use instanceof
with the appropriate type interface.
Comment | File | Size | Author |
---|---|---|---|
#24 | remove-gettype-2056721-24.patch | 16.41 KB | effulgentsia |
#24 | interdiff.txt | 2.39 KB | effulgentsia |
#21 | remove-gettype-2056721-21.patch | 16.31 KB | effulgentsia |
#21 | interdiff.txt | 9.68 KB | effulgentsia |
#6 | remove-gettype-2056721-6.patch | 9.1 KB | Berdir |
Comments
Comment #1
BerdirLooks like removing it might be possible. Let's try this.
Comment #2
yched CreditAttribution: yched commentedIf not, I'd suggest at leat renaming it to getDataType() - same old "interfaces / base classes that are supposed to exist across a wide range of business objects should namespace their methods to avoid collision / confusion" argument we have applied to a couple other places now :-)
Comment #4
BerdirOf all places, it was text item that used that method :p
Comment #6
BerdirFixed the config test. I think the other one was a random fail.
Comment #7
yched CreditAttribution: yched commentedLooks good, but it should probably be RTBCed by someone more familiar with TypedData than I am :-/
Comment #8
xjm"To be filled out later" :P
Here's a start for ya:
API changes
The
TypedDataInterface::getType()
method is removed. Callers should instead useinstanceof
with the appropriate type interface.Comment #9
xjmThanks!
Comment #10
msonnabaum CreditAttribution: msonnabaum commentedNot to expand the scope here, but the same argument can be made for some more of TypedDataInterface's methods.
In particular:
- getName
- getParent
- getDefinition
Comment #11
xjmSo this is soft-blocking #2049039: Convert node properties to methods. based on @webchick's most comment there. Bumping priority accordingly.
Comment #12
amateescu CreditAttribution: amateescu commentedI agree with #10 and would also like to add getRoot() to that list :)
Comment #13
Berdir@msonnabaum/amateescu: This issue is just about this method. A separate issue to remove the interface completely was opened and worked on in #2002138: Use an adapter for supporting typed data on ContentEntities but not ready in time for API freeze, so I'm not sure how much of that is still possible, that's why I started doing this in a separate step.
Comment #14
effulgentsia CreditAttribution: effulgentsia commentedI see the removal of these tests with no equivalent replacement of them to something else as problematic.
IMO, this is an even worse abstraction leak. Now we're not only leaking the TypedData abstraction, but also that TypedData is implemented as plugins.
Yeah, I'd prefer this issue's scope to do that instead. Removing the interface entirely is a job for #2002138: Use an adapter for supporting typed data on ContentEntities. If we manage to do that, great. But if not, then IMO, having the interface, but without its key method is a worse WTF than having it as a complete interface.
Comment #15
xjmBut aren't they just unit tests for the removed method? Though, it doesn't matter if we go with renaming the method instead for now in order to unblock #2049039: Convert node properties to methods..
Comment #16
effulgentsia CreditAttribution: effulgentsia commentedSorry I misread the patch. The two lines in ConfigSchemaTest are actually replaced by calls to getPluginId(), so those are just two more examples of my 2nd comment from #14, that I think getPluginId() is a much worse interface for getting the type than a getDataType() method would be.
The TypedDataTest hunk, however, is removing the test for ensuring that the created object is of the type that was requested.
Comment #17
BerdirHm. Typed data classes are plugins. getType() returns the same information as getPluginId(), so not sure if it's a bad idea to remove it or not. You will have to know hat they are plugins if you want to mess with it sooner or later.
The config stuff for example just hasn't been updated after the interfaces issue, which made sure that every basic type has a class and an interface. And in TypedDataTest calls the plugin manager to get a typed data plugin.. why would getPluginId() be bad there (yes should not have removed it).
But I'm not sure either :)
Comment #18
BerdirUps.
Comment #19
fagoYes, but typed data plugins are quite generic - as typed data. E.g. if field item is based on a field type plugin which is exposed as typed data plugin as well, so which plugin ID do I get witih getPluginId()? I guess it's more a coincidence it's the data type plugin ID and not the field plugin ID?
I think this should be able to check the field type - a pity it's not available for non-configurable field yet. ( #2047229: Make use of classes for entity field and data definitions would add that.)
That makes a lot of sense, however the text-with-summary example shows where this goes: For checking the text with summary field type we'd need an interface for that. While this is one thing more folks would have to declare, I think it would make sense:
We need the possibility to swap out the plugin class - what is fine - but then we need an interface for checking the plugin identity. So this boils down to a general getPluginId() vs interface check question, with the interface-check having some advantages:
- it leverages language features instead inventing our own variant
- it works with a class being of multiple plugin types at the same time (field type + data type)
- it works with inheritance - you can extend and customize plugin without loosing the plugin "identity", e.g. think of entity reference fields and their variations
Surely, it's not feasible to require interfaces for all plugins at this point. But maybe we can at least encourage defining interfaces for data types and do so for all core field and entity types (checked)? Also see #2028097: Map data types to interfaces to make typed data discoverable.
Side effect: Having field type interfaces would be very useful to improve widget,formater <-> field type compatibility as we could expand supported types based upon interfaces...
Comment #20
yched CreditAttribution: yched commentedHm - that's a stretch :-). Also, see that discussion about inheritance between field type plugins & widget plugins:
#2050113: PHP notice on multiple items image field (#6 / #7)
Comment #21
effulgentsia CreditAttribution: effulgentsia commentedHonestly, I'd still prefer us to just rename to getDataType(), and punt on removing the interface to #2002138: Use an adapter for supporting typed data on ContentEntities. But, seems like others here are ok with crippling the interface as an interim step to removing it. Given that, here's a patch that does that in a way that incorporates #14 and #19.
Comment #22
effulgentsia CreditAttribution: effulgentsia commentedJust to clarify, the reason for this change is that within config schema, the 'label' and 'path' types are just extensions of 'string'. They don't have their own interface or class. They just serve as a way of providing some default definition values; most importantly, the 'translatable' one.
Comment #23
BerdirThis looks... interesting :) Seems like we're hijacking a method that's defined by an interface.
Like the idea, but I'd either use a property for this or rely on $this->getProperties(), isn't that kind of why we have it? :)
Looking at this and getProperties(), I'm a bit confused though. getProperties() defines processed and summary_processed. What's the difference between those? Because processed actually has settings that define the text source, so that would allow us to loop over them?
Comment #24
effulgentsia CreditAttribution: effulgentsia commentedLike this?
Comment #25
yched CreditAttribution: yched commentedI have nothing against this specific refactor in TextItemBase, but I'd very much regret if a FieldItem plugin class cannot access its own plugin id anymore ?
Comment #26
Berdir@yched: What do you consider "its own plugin id"? What it was checking before was he typed data plugin id, *not* the field type plugin id. We're basically exposing a single class as two different plugin implementations and types, and the plugin id that it returns solely depends on which manager happens to instantiate it... which is.... crazy. But it was already like that before, this isn't introduced here.
Comment #27
effulgentsia CreditAttribution: effulgentsia commentedI don't think that's so crazy. The point of a plugin id is to identify the plugin within the scope of a manager. There shouldn't be that much code that needs to call getPluginId(). Any code that does should be ok with it being potentially variable based on which manager is managing that plugin. This is not unlike $node->id() returning a different id on dev vs. prod.
It still can, subject to what's noted above. More commonly, however, a FieldItem doesn't need to know its own plugin id, but rather its field type. And it can get that via $this->getFieldDefinition()->getFieldType(). If we want, we could add a protected getFieldType() method to FieldItemBase that wraps that, just like we have for getFieldSetting(). But even that, I think, shouldn't be all that common, since why do you need to know which type you are? Typically, it's because there's some functionality that needs to differ by type, but that should be implemented via PHP's OOP techniques: classes and interfaces. I agree, though, that even if getFieldType() and getPluginId() aren't needed very commonly, that they still need to exist.
Comment #28
yched CreditAttribution: yched commented[edit: crosspost with @eff #27]
I know :-/, that's the craziness of "FieldItem classes are discovered through a plugin type, but instantiated through another plugin type, and we statically cache both separate lists of definitions in two different plugin managers"...
So yeah, code in a FieldItem class checking its own type (plugin id) relies on the fact that FieldItem classes are only ever instantiated through the DataType manager, so the plugin id is the one "as a data type".
I agree that it's insane+confusing and would very much like to get rid of that "class is exposed as one plugin type but used as another plugin type" too...
It's a direct consequence of "one TypedDatainterface + one single bag of 'data type definitions' to rule them all", tough ("all" = fields, field items, properties...). IMO we'd be better off with separate plugin types, one for "entity fields", one for "primitive data / properties"...
I don't know to which extend the ongoing issues to reduce the hegemony of TypedData will allow or include that already.
Comment #29
yched CreditAttribution: yched commentedAnd agreed with @effulgentsia #27 that proper OO is better than self-inspecting the plugin type :-)
Comment #30
BerdirOk, so yeah, relying on classes/interfaces or the getFieldType() method is I think preferred over getPluginId() or getType() anyway, and the plugin ID crazyness isn't introduced here.
I'm +1 on the latest patch, I'll try to get @fago to RTBC it tomorrow, it's blocking progress on my node issues. I *think* we're in line with what he said, so if @yched want's to RTBC, that works for me too :)
The only nitpick that I have is that I'm not sure if TextProcessed in the comments needs to be namespaces or not, still don't completely get our rules :)
Comment #31
yched CreditAttribution: yched commentedHm, sorry @Berdir, I'm fine with the changes in text.module, but I'd feel more comfortable letting @fago RTBC the rest :-/
Comment #32
fagoPatch looks good to me, just one thing:
I don't think this has to as getDefinition() is still here. Instead of $data->getType() we can just use $definition['type'] for the second hunk also.
Comment #33
effulgentsia CreditAttribution: effulgentsia commentedWe can, but should we? If the claim in this issue is that we should get rid of getType() instead of renaming it, then the only way that claim makes sense is if we're saying that consuming code should never care about getting the TypedData type. If it does care, we should have a method for it (e.g., getDataType()), not make consuming code get it out of the definition. The arguments so far for getting rid of the concept of a TypedData data type from consuming code is that we have PHP interfaces instead, so that's what #24 replaces that hunk with, but does it from the caller of that method, since only the caller knows what interface to check.
Comment #34
effulgentsia CreditAttribution: effulgentsia commentedComment #35
webchickberdir and I already talked about this, and it's necessary to resolve a critical.
Comment #36
Berdir#24: remove-gettype-2056721-24.patch queued for re-testing.
Comment #37
BerdirI think I agree with @effulgentsia here, the type check has been replaced with interface checks. If we'd already have the interface in the definition, we could do that in a generic way but we don't have that yet. And it's also more explicit that way, as the definition could contain whatever it want, a hardcoded check makes kind of sense.
I worked on this patch too, so not comfortable with RTBC, someone else up for it? According to @fago in #32, that was the only questionable thing, he is currently in holidays I think and it's so trivial that we could easily re-add if we want later on.
Comment #38
msonnabaum CreditAttribution: msonnabaum commentedLooks good to me.
Comment #39
webchick#24: remove-gettype-2056721-24.patch queued for re-testing.
Comment #40
Dries CreditAttribution: Dries commentedReviewed this and it looks good. Waiting for the testbot to come back.
Comment #41
effulgentsia CreditAttribution: effulgentsia commentedBot is back and green.
Comment #42
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks!
Comment #43
chx CreditAttribution: chx commentedThe change is great I have on occasion used getType cos it was more logical than entityType and of course that was the wrong to do so this is a great DX improvement.
Comment #44
BerdirShould have updated the issue title before commit ;)
Change notice: https://drupal.org/node/2066763
Comment #45
Gábor HojtsyAll right, the getType() method is used in config_inspector to inspect the configuration typed data structure. Now this issue breaks 90% of config_inspector (listing CMI keys still works and the raw data dump works, but not any of the other representations of the data because they rely on data type introspection). See #2066997: Form inspector shows undefined method DataType\Boolean::getType() error. That code cannot use instanceof at all because it does not even know what kind of types are possible. Typed data is designed to be extensible. Neither the change notice, nor the API change summary above gives any guidance as to what to do with this now...
How would this code get the type now to expose?
Comment #46
BerdirYou still have the type key in $definition. We didn't remove that. And getPluginId() should give you the same as getType(), as typed data are plugin implementations. Which is IMHO valid to use as you are explicitly working with typed data plugins in that case.
Your example code just uses the type for debug output. That's IMHO a valid use case for relying on the plugin id/definition. For actual logic (like deciding if something is translatable or not), an interface would be preferable over that.
Not exactly sure what to write into the change notice, let's discuss...
Make sure you follow #2002138: Use an adapter for supporting typed data on ContentEntities though, as we intend to remove/move more of that interface and need to make sure we cover uses cases like yours.
Comment #47
Gábor HojtsyRight, @juanolalla is smarter than me and already figured out we can use the definition key :D I've added this to the change notice:
Comment #48.0
(not verified) CreditAttribution: commentedUpdated issue summary.