PathItem doesn't return correct main property from mainPropertyName()

Makes total sense, we should add a tiny unit test that asserts the output of this method.

Oh yes, I'd like to see such a test. How would you make sure you catch all possible fields provided by all possible modules in core?

Related: #2681241: Path doesn't behave as a regular field and #2539634: PathItem::delete() never runs because the path field type is a computed field in disguise.

Re-titling in an attempt to understand the issue. This isn't about documentation, but returning the name of it's main property via the API.

Er, it doesN'T do that :-)

Rerolled the tests at #7.

#7: "path doesn't seem to show up there... probably because path module's not on. But even this test right now shows we have problems."

Adding path to the enabled modules ensures those errors are discovered.

Edited: Also modified the tests so we cover cases with no properties defined, as the MapItem does.

Also fixed ShapeItem while we are here, so the tests are green after the patch. I'm wondering how could we ensure that at least all core modules are added, so we check all the field type plugins.

1. +++ b/core/modules/field/tests/src/Kernel/FieldTypePluginManagerTest.php
   @@ -13,6 +13,14 @@
   +  /**
   +   * Modules to enable.
   +   *
   +   * @var array
   +   */
   

   Let's use {@inheritdoc}

2. +++ b/core/modules/field/tests/src/Kernel/FieldTypePluginManagerTest.php
   @@ -13,6 +13,14 @@
   +  public static $modules = ['user', 'system', 'field', 'text', 'entity_test', 'field_test', 'path'];
   

   what about file and image, just to be sure. $this->enableModules

3. +++ b/core/modules/field/tests/src/Kernel/FieldTypePluginManagerTest.php
   @@ -84,4 +92,19 @@ public function testCreateInstanceWithConfig() {
   +  public function testMainProperty() {
   

   Super cool idea!

4. +++ b/core/modules/field/tests/src/Kernel/FieldTypePluginManagerTest.php
   @@ -84,4 +92,19 @@ public function testCreateInstanceWithConfig() {
   +        $message = SafeMarkup::format("@plugin property @property found in @properties", ['@plugin' => $plugin_id, '@property' => $property, '@properties' => $properties]);
   

   we use sprintf / literal php strings now.

Let's try enabling all modules then.

Cool, thank you!

FQCN (see interdiff)

Closed #2622300: PathItem must override mainPropertyName() as duplicated, it's older but tests are much better here.

+++ b/core/modules/field/tests/src/Kernel/FieldTypePluginManagerTest.php
@@ -84,4 +86,29 @@ public function testCreateInstanceWithConfig() {
+    $module_list = array_filter(array_keys($module_list), function($module) use ($module_handler) {
+      return !$module_handler->moduleExists($module);
+    });

This should be limited to core modules only. Test modules should be chosen to be enabled. I was wondering if this includes test modules too - imho test modules shouldn't be enabled here either.

1. +++ b/core/modules/field/tests/src/Kernel/FieldTypePluginManagerTest.php
   @@ -3,6 +3,8 @@
   +use Drupal\Core\Extension\ModuleHandlerInterface;
   

   Not used.

2. +++ b/core/modules/field/tests/src/Kernel/FieldTypePluginManagerTest.php
   @@ -84,4 +86,29 @@ public function testCreateInstanceWithConfig() {
    
   +  public function testMainProperty() {
   

   Needs a docblock.

3. +++ b/core/modules/system/tests/modules/entity_test/src/Plugin/Field/FieldType/ShapeItem.php
   @@ -77,4 +77,12 @@ public function isEmpty() {
   +    return 'color';
   

   Not shape?

#23: Tests module were being installed. Filtering now that only core modules and non-testing modules are installed. The only way I knew for filtering those is looking at their path. Please suggest a better way if any.

#24: Fixed.

Re-testing, that doesn't look related.

Back to needs review then :-)

1. +++ b/core/modules/field/tests/src/Kernel/FieldTypePluginManagerTest.php
   @@ -84,4 +85,33 @@ public function testCreateInstanceWithConfig() {
   +    $listing = new ExtensionDiscovery(\Drupal::root());
   +    $module_list = $listing->scan('module', FALSE);
   +    /** @var \Drupal\Core\Extension\ModuleHandlerInterface $module_handler */
   +    $module_handler = $this->container->get('module_handler');
   +    $module_list = array_filter(array_keys($module_list), function($module) use ($module_handler, $module_list) {
   +      return !$module_handler->moduleExists($module) && substr($module_list[$module]->getPath(), 0, 4) === 'core';
   +    });
   +    $this->enableModules($module_list);
   

   Let's move that into its own method

2. +++ b/core/modules/field/tests/src/Kernel/FieldTypePluginManagerTest.php
   @@ -84,4 +85,33 @@ public function testCreateInstanceWithConfig() {
   +        $this->assertTrue(in_array($property, array_keys($class::propertyDefinitions($storage_definition))), $message);
   

   We can use $this->assertArrayKeyExists or so

Here you are.

Ooopsie

Re-testing, looks like #2724871: Random failure in \Drupal\migrate_drupal_ui\Tests\d7\MigrateUpgrade7Test

It was @penyaskito!

Committed 8f61056 and pushed to 8.1.x. Thanks!