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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Status: Active » Needs review
Issue tags: +Entity Field API, +Typed sanity
FileSize
5.64 KB

Looks like removing it might be possible. Let's try this.

yched’s picture

If 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 :-)

Status: Needs review » Needs work

The last submitted patch, remove-gettype-2056721-1.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
858 bytes
6.48 KB

Of all places, it was text item that used that method :p

Status: Needs review » Needs work

The last submitted patch, remove-gettype-2056721-4.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
2.62 KB
9.1 KB

Fixed the config test. I think the other one was a random fail.

yched’s picture

Looks good, but it should probably be RTBCed by someone more familiar with TypedData than I am :-/

xjm’s picture

"To be filled out later" :P

Here's a start for ya:

API changes

The TypedDataInterface::getType() method is removed. Callers should instead use instanceof with the appropriate type interface.

xjm’s picture

Thanks!

msonnabaum’s picture

Not to expand the scope here, but the same argument can be made for some more of TypedDataInterface's methods.

In particular:

- getName
- getParent
- getDefinition

xjm’s picture

Priority: Normal » Major
Issue tags: +blocker

So this is soft-blocking #2049039: Convert node properties to methods. based on @webchick's most comment there. Bumping priority accordingly.

amateescu’s picture

I agree with #10 and would also like to add getRoot() to that list :)

Berdir’s picture

@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.

effulgentsia’s picture

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigSchemaTest.php
@@ -162,13 +164,13 @@ function testSchemaData() {
-    $this->assertEqual($property->getType(), 'label', 'Got the right string type for site name data.');
...
+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigSchemaTest.php
@@ -162,13 +164,13 @@ function testSchemaData() {
-    $this->assertEqual($property->getType(), 'path', 'Got the right type for page.front data (undefined).');
...
+++ b/core/modules/system/lib/Drupal/system/Tests/TypedData/TypedDataTest.php
@@ -52,16 +52,12 @@ public function setUp() {
-    // Assert that the correct type was constructed.
-    $this->assertEqual($data->getType(), $type, format_string('!type object returned type.', array('!type' => $definition['type'])));

I see the removal of these tests with no equivalent replacement of them to something else as problematic.

+++ b/core/modules/text/lib/Drupal/text/Plugin/field/field_type/TextItemBase.php
@@ -79,7 +79,7 @@ public function prepareCache() {
-      if ($this->getType() == 'field_item:text_with_summary') {
+      if ($this->getPluginId() == 'field_item:text_with_summary') {

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.

I'd suggest at leat renaming it to getDataType()

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.

xjm’s picture

I see the removal of these tests with no equivalent replacement of them to something else as problematic.

But 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..

effulgentsia’s picture

But aren't they just unit tests for the removed method?

Sorry 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.

Berdir’s picture

Priority: Major » Normal

Hm. 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 :)

Berdir’s picture

Priority: Normal » Major

Ups.

fago’s picture

Hm. 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.

Yes, 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?

-      if ($this->getType() == 'field_item:text_with_summary') {
+      if ($this->getPluginId() == 'field_item:text_with_summary') {

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.)

The TypedDataInterface::getType() method is removed. Callers should instead use instanceof with the appropriate type interface.

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...

yched’s picture

Having field type interfaces would be very useful to improve widget,formater <-> field type compatibility as we could expand supported types based upon interfaces.

Hm - 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)

effulgentsia’s picture

Honestly, 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.

effulgentsia’s picture

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigSchemaTest.php
@@ -162,14 +164,16 @@ function testSchemaData() {
-    $this->assertEqual($property->getType(), 'label', 'Got the right string type for site name data.');
+    $definition = $property->getDefinition();
+    $this->assertTrue($definition['translatable'], 'Got the right translatability setting for site name data.');
 
-    $this->assertEqual($property->getType(), 'path', 'Got the right type for page.front data (undefined).');
+    $definition = $property->getDefinition();
+    $this->assertTrue(empty($definition['translatable']), 'Got the right translatability setting for page.front data.');

Just 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.

Berdir’s picture

+++ b/core/modules/text/lib/Drupal/text/Plugin/field/field_type/TextItemBase.phpundefined
@@ -70,7 +70,7 @@ public function isEmpty() {
-  public function prepareCache() {
+  public function prepareCache($properties = array('value')) {

@@ -78,9 +78,8 @@ public function prepareCache() {
-      $this->set('safe_value', text_sanitize($text_processing, $langcode, $itemBC, 'value'));
-      if ($this->getType() == 'field_item:text_with_summary') {
-        $this->set('safe_summary', text_sanitize($text_processing, $langcode, $itemBC, 'summary'));
+      foreach ($properties as $name) {
+        $this->set("safe_$name", text_sanitize($text_processing, $langcode, $itemBC, $name));

This 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?

effulgentsia’s picture

Like this?

yched’s picture

I 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 ?

Berdir’s picture

@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.

effulgentsia’s picture

and the plugin id that it returns solely depends on which manager happens to instantiate it... which is.... crazy

I 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.

I'd very much regret if a FieldItem plugin class cannot access its own plugin id anymore

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.

yched’s picture

[edit: crosspost with @eff #27]

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

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.

yched’s picture

And agreed with @effulgentsia #27 that proper OO is better than self-inspecting the plugin type :-)

Berdir’s picture

Ok, 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 :)

yched’s picture

Hm, sorry @Berdir, I'm fine with the changes in text.module, but I'd feel more comfortable letting @fago RTBC the rest :-/

fago’s picture

Status: Needs review » Needs work

Patch looks good to me, just one thing:

@@ -47,21 +47,13 @@ public function setUp() {
-    $this->assertEqual($data->getType(), $type, format_string('!type object returned type.', array('!type' => $definition['type'])));

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.

effulgentsia’s picture

we can just use $definition['type'] for the second hunk

We 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.

effulgentsia’s picture

Status: Needs work » Needs review
webchick’s picture

Issue tags: +Approved API change

berdir and I already talked about this, and it's necessary to resolve a critical.

Berdir’s picture

#24: remove-gettype-2056721-24.patch queued for re-testing.

Berdir’s picture

I 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.

msonnabaum’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

webchick’s picture

#24: remove-gettype-2056721-24.patch queued for re-testing.

Dries’s picture

Reviewed this and it looks good. Waiting for the testbot to come back.

effulgentsia’s picture

Bot is back and green.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks!

chx’s picture

Title: Remove or rename TypedDataInterface::getType() » Change notice: Remove or rename TypedDataInterface::getType()
Priority: Major » Critical
Status: Fixed » Active

The 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.

Berdir’s picture

Title: Change notice: Remove or rename TypedDataInterface::getType() » Remove TypedDataInterface::getType()
Priority: Critical » Major
Status: Active » Fixed

Should have updated the issue title before commit ;)

Change notice: https://drupal.org/node/2066763

Gábor Hojtsy’s picture

Title: Remove TypedDataInterface::getType() » Change notice: Remove TypedDataInterface::getType()
Priority: Major » Critical
Status: Fixed » Active
Issue tags: +Needs change record

All 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?

/**
 * Format config schema as a tree.
 */
function config_inspector_format_tree($schema, $collapsed = FALSE, $base_key = '') {
  $build = array();
  foreach ($schema as $key => $element) {
    $type = $element->getType();
    $definition = $element->getDefinition() + array('label' => t('N/A'));
    $element_key = $base_key . $key;
    if ($element instanceof Element) {
      $build[$key] = array(
        '#type' => 'details',
        '#title' => $definition['label'],
        '#description' => $element_key . ' (' . $type . ')',
        '#collapsible' => TRUE,
        '#collapsed' => $collapsed,
      ) + config_inspector_format_tree($element, TRUE, $element_key . '.');
    }
    else {
      $value = $element->getString();
      $build[$key] = array(
        '#type' => 'item',
        '#title' => $definition['label'],
        '#markup' => check_plain(empty($value) ? t('<empty>') : $value),
        '#description' => $element_key . ' (' . $type . ')',
      );
    }
  }
  return $build;
}
Berdir’s picture

You 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.

Gábor Hojtsy’s picture

Title: Change notice: Remove TypedDataInterface::getType() » Remove TypedDataInterface::getType()
Priority: Critical » Major
Status: Active » Fixed
Issue tags: -Needs change record

Right, @juanolalla is smarter than me and already figured out we can use the definition key :D I've added this to the change notice:

If you need to introspect types (and so you cannot use instanceof), see the 'type' key in $definition. The getPluginId() method can similarly be useful to get type information.

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.