Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plopesc’s picture

Assigned: Unassigned » plopesc

Working on this one.
Regards

plopesc’s picture

Status: Active » Needs review
FileSize
35.44 KB

First round. Let's see it if testbot is green.

Given that the issue title refers only to uses of these function, I didn't remove the function declaration in patch. Is it right?

This new implementation of field_info_*_settings() could imply some problem, because is maybe necessary to check if the 'settings' property exist each time settings are requested. Previously, the function returned an empty array if that property wasn't set .

Regards.

Status: Needs review » Needs work

The last submitted patch, remove_field_info_types_settings-2047993-2.patch, failed testing.

tstoeckler’s picture

@plopesc. Since removing the functions would be an API change, it's debatable whether it's still acceptable. I personally think that removing old and deprecated APIs is part of the clean-up phase und would make sense.

plopesc’s picture

Status: Needs work » Needs review
FileSize
39.08 KB

Re-rolling patch removing deprecated API functions. Running testbot again.

yched’s picture

Status: Needs review » Needs work
Issue tags: -API change

Thanks @plopesc!

Yes, let's leave the functions around for now, we'll check committers feedback to see if the removal is acceptable.

+    $formatter_settings =  \Drupal::service('plugin.manager.field.formatter')->getDefinition($default_formatter);
     $expected = array(
        // ...
-      'settings' => $default_settings,
+      'settings' => $formatter_settings['settings'],

I was thinking we should add a getDefaultSettings($plugin_id) method to the (Formatter|Widget)PluginManager classes, that would directly return the 'settings' entry of the definition :if ($definition = $this->getDefinition($plugin_id)) { return $definition['settings']; } else { return array(); }.
For the FieldTypePluginManager, we would need two methods: getDefaultFieldSettings() (checks 'settings'), getDefaultInstanceSettings() (checks 'instance_settings').
We would then need to update the @deprecated mentions for the old field_info_*_settings() functions to point to those new methods.

Then, a non-completely trivial thing is that, according to D8 practices about "dependency injection", we want to avoid getting the plugin managers by using \Drupal::service() inside classes whenever possible, and instead get them injected in the object __construct() using the DIC.
That's not always possible; for example, entity classes (or entity controller classes) currently have no way to receive injected services, they need to pull them using \Drupal::service().

But Drupal\edit\EditorSelector, for example, is a service - it has an entry in edit.services.yml. So it can receive its dependencies injected:
- add '@plugin.manager.field.formatter' to its 'arguments' entry in the yml file
- add a FormatterPluginManager $formatter_manager argument to EditorSelector::__construct(), and store it in a new $this->formatterManager class property (will need to be documented next to "protected $editorManager" at the beginning of the class).
- then the code can do $formatter_info = $this->formatterManager->getDefinition($formatter_type);

Going through the patch, this also applies to the following services:
- FieldInfo (needs the plugin.manager.entity.field.field_type service injected)
- FormatterPluginManager (ibid.)
- WidgetPluginManager (ibid.)

Then we have the following page controllers, those work slightly differently, they pull their dependencies in a static create() factory method:
- FieldOverview (uses plugin.manager.entity.field.field_type)
override create() and make it return new static($container->get('plugin.manager.entity'), $container->get('plugin.manager.entity.field.field_type'));
override __construct() and add a $field_type_manager parameter and a $this->fieldTypeManager property
- FieldInstanceEditForm
same principle, but you can modify the create() and __construct() methods directly in FieldInstanceFormBase

diff --git a/core/modules/field/lib/Drupal/field/Plugin/Type/Formatter/FormatterPluginManager.php
@@ -142,11 +142,12 @@ public function prepareConfiguration($field_type, array $configuration) {
     // Fill in default settings values for the formatter.
-    $configuration['settings'] += field_info_formatter_settings($configuration['type']);
+    $formatter = \Drupal::service('plugin.manager.field.formatter')->getDefinition($configuration['type']);
+    $configuration['settings'] += $formatter['settings'];

No need to re-pull the FormatterPluginManager from itself, it's $this ;-) (hem, which shows how silly the current code is...)
Same in WidgetPluginManager.

yched’s picture

Status: Needs review » Needs work

@plopesc: Sorry for the giant feedback. I hope it doesn't discourage you, your help around the FIeld queue is *really* helpful and appreciated! We'll do our best to stick around and help :-)

yched’s picture

Status: Needs work » Needs review
Issue tags: +API change

Oops, #6 / #7 are crossposts with #5.

OK, never mind for the functions, keep them removed since you have done the work, we'll ask for committers feedback.

plopesc’s picture

@yched: No worries, I was sure that it wouldn't be so easy and some rerolls would be necessary :)
I've been digging the (Formatter|Widget)PluginManager classes and both provide the getDefaultSettings method. Then, we can use it and implement only getDefaultFieldSettings(), getDefaultInstanceSettings() in FieldTypePluginManager class.

Thanks for your feedback, I'll do my best!

yched’s picture

I've been digging the (Formatter|Widget)PluginManager classes and both provide the getDefaultSettings method

Hah, right, I skipped that. Must've been added while I was away for a while. Sweet :-)

plopesc’s picture

Status: Needs work » Needs review
FileSize
17.23 KB
37.5 KB

Hello
New round. This patch tries to address some of @yched comments.

- Creates new methods for FieldTypePluginManager
- Modifies docs and functions in field.info.inc file according to methods implemented in FieldTypePluginManager
- Call $this instead of re-pull managers.

I think it could be green now after some testing in my side. Then I should start to improve it and use Dependency Injection.

Regards

yched’s picture

Thanks !

+++ b/core/lib/Drupal/Core/Entity/Field/FieldTypePluginManager.phpundefined
@@ -54,4 +54,34 @@ public function __construct(\Traversable $namespaces, CacheBackendInterface $cac
+   * Returns the default settings of a field widget.

(phpdoc for getDefaultSettings())
"... the default field-level settings for a field type"

Nitpick: I'd probably advocate for getDefaultFieldSettings().
Having getDefaultSettings() & getDefaultInstanceSettings() kind of "hides" the second one. Those are equally important concepts.

+++ b/core/lib/Drupal/Core/Entity/Field/FieldTypePluginManager.phpundefined
@@ -54,4 +54,34 @@ public function __construct(\Traversable $namespaces, CacheBackendInterface $cac
+   * Returns the default settings of a field widget.

(phpdoc for getDefaultInstanceSettings())
"... the default instance-level settings for a field type"

+++ b/core/lib/Drupal/Core/Entity/Field/FieldTypePluginManager.phpundefined
@@ -54,4 +54,34 @@ public function __construct(\Traversable $namespaces, CacheBackendInterface $cac
+   *   A field instance name.

(phpdoc for getDefaultInstanceSettings())
"A field type name"

+++ b/core/modules/field/lib/Drupal/field/Tests/FieldInfoTest.phpundefined
@@ -279,9 +279,8 @@ function testFieldMap() {
+      $this->assertIdentical(\Drupal::service('plugin.manager.entity.field.field_type')->getDefaultSettings($type), $data['settings'], format_string("field settings service returns %type's field settings", array('%type' => $type)));
+      $this->assertIdentical(\Drupal::service('plugin.manager.entity.field.field_type')->getDefaultInstanceSettings($type), $data['instance_settings'], format_string("field instance settings service returns %type's field instance settings", array('%type' => $type)));

Minor: at this point, better extract to a variable to have shorter lines

plopesc’s picture

New patch

Includes improvements suggested in #12 and dependency injection proposed in #5. Also includes DIC support for core/modules/field_ui/lib/Drupal/field_ui/FieldListController.php class.

Regards.

yched’s picture

Looks good, thanks !

Only minor phrasing nitpicks:

+++ b/core/modules/edit/lib/Drupal/edit/EditorSelector.phpundefined
@@ -35,9 +43,12 @@ class EditorSelector implements EditorSelectorInterface {
    * @param \Drupal\Component\Plugin\PluginManagerInterface
    *   The manager for editor plugins.
+   * @param \Drupal\field\Plugin\Type\Formatter\FormatterPluginManager
+   *   The formatter manager.

Then for consistency with the comment above, it should be "The manager for formatter plugins".

+++ b/core/modules/field/lib/Drupal/field/FieldInfo.phpundefined
@@ -45,6 +46,13 @@ class FieldInfo {
+   * The field type manager to define field.

"The 'field type' plugin manager".

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Formatter/FormatterPluginManager.phpundefined
@@ -31,6 +32,13 @@ class FormatterPluginManager extends DefaultPluginManager {
+   * The field type manager to define field.

same

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Widget/WidgetPluginManager.phpundefined
@@ -24,6 +25,13 @@
+   * The field type manager to define field.

same

Status: Needs review » Needs work

The last submitted patch, remove_field_info_types_settings-2047993-13.patch, failed testing.

plopesc’s picture

Status: Needs work » Needs review
FileSize
7.84 KB
53.12 KB

Sorry

I forgot to review Editor test files. Reviewing these files, I found in those tests that services are loaded via $this->container->get instead of \Drupal::service() method.

I followed this approach in these files. Which type of load should we use? Should we change the implementation in the other test files?

Regards.

yched’s picture

I don't even think I was aware of $this->container in tests :-)
It seems we're using both syntaxes in core tests right now, with no clear dominant trend, so I'd say the patch is fine as is.

tim.plunkett’s picture

$this->container is preferred in tests, but it makes little difference.

yched’s picture

Status: Needs review » Reviewed & tested by the community

OK, green and @tim.plunkett's #18: RTBC then.
Thanks !

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a reroll...

git ac https://drupal.org/files/remove_field_info_types_settings-2047993-16.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 54398  100 54398    0     0  22425      0  0:00:02  0:00:02 --:--:-- 24503
error: core/modules/field_ui/lib/Drupal/field_ui/Form/FieldInstanceFormBase.php: does not exist in index
error: patch failed: core/modules/options/lib/Drupal/options/Plugin/field/widget/OptionsWidgetBase.php:115
error: core/modules/options/lib/Drupal/options/Plugin/field/widget/OptionsWidgetBase.php: patch does not apply
plopesc’s picture

Status: Needs work » Needs review
FileSize
51.08 KB

Rerolling...

yched’s picture

Yeah, FieldInstanceFormBase has been removed in #2050367: FieldInstanceFormBase is useless, the code that was added in there needs to go directly in FieldInstanceEditForm now.

from #16:

diff --git a/core/modules/field_ui/lib/Drupal/field_ui/Form/FieldInstanceFormBase.php b/core/modules/field_ui/lib/Drupal/field_ui/Form/FieldInstanceFormBase.php
index 5f7ca61..8f713b0 100644
--- a/core/modules/field_ui/lib/Drupal/field_ui/Form/FieldInstanceFormBase.php
+++ b/core/modules/field_ui/lib/Drupal/field_ui/Form/FieldInstanceFormBase.php
@@ -10,6 +10,7 @@
 use Drupal\Core\Form\FormInterface;
 use Drupal\Core\Controller\ControllerInterface;
 use Drupal\Core\Entity\EntityManager;
+use Drupal\Core\Entity\Field\FieldTypePluginManager;
 use Drupal\field\Plugin\Type\Widget\WidgetPluginManager;
 use Drupal\field\FieldInstanceInterface;
 use Drupal\field_ui\FieldUI;
@@ -39,16 +40,26 @@
   protected $entityManager;
 
   /**
+   *  The field type manager.
+   *
+   * @var \Drupal\Core\Entity\Field\FieldTypePluginManager
+   */
+  protected $fieldTypeManager;
+
+  /**
    * Constructs a new field instance form.
    *
    * @param \Drupal\Core\Entity\EntityManager $entity_manager
    *   The entity manager.
    * @param \Drupal\field\Plugin\Type\Widget\WidgetPluginManager $widget_manager
    *   The field widget plugin manager.
+   * @param \Drupal\Core\Entity\Field\FieldTypePluginManager $field_type_manager
+   *   The field type manager
    */
-  public function __construct(EntityManager $entity_manager, WidgetPluginManager $widget_manager) {
+  public function __construct(EntityManager $entity_manager, WidgetPluginManager $widget_manager ,FieldTypePluginManager $field_type_manager) {
     $this->entityManager = $entity_manager;
     $this->widgetManager = $widget_manager;
+    $this->fieldTypeManager = $field_type_manager;
   }
 
   /**
@@ -57,7 +68,8 @@ public function __construct(EntityManager $entity_manager, WidgetPluginManager $
   public static function create(ContainerInterface $container) {
     return new static(
       $container->get('plugin.manager.entity'),
-      $container->get('plugin.manager.field.widget')
+      $container->get('plugin.manager.field.widget'),
+      $container->get('plugin.manager.entity.field.field_type')
     );
   }

#21 doesn't seem to do this, so I don't think it will work.

plopesc’s picture

Sorry, forgot to review this class :(
Injecting FieldTypeManager in FieldInstanceEditForm now.

yched’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!
Looks good if comes back green.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

YesCT’s picture

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldOverview.phpundefined
@@ -17,6 +19,36 @@
+  public function __construct(EntityManager $entity_manager, FieldTypePluginManager $field_type_manager) {
+    $this->entityManager = $entity_manager;

in #2045043-17: Field listings operations cannot be altered we did parent::__construct($entity_manager);

kinda cool.

plopesc’s picture

Status: Fixed » Needs review
Issue tags: -Needs reroll
FileSize
651 bytes

Attaching change suggested by YesCT.

Regards.

yched’s picture

Status: Needs review » Fixed
Issue tags: +Needs reroll
yched’s picture

Issue tags: -Needs reroll

tag slipped in.

rahul.metallica’s picture

Status: Fixed » Needs review
yched’s picture

Status: Needs review » Fixed

Nonono, this is fixed :-)
The main patch has been committed in #25, and the changes in #27 have been taken care of in #2045043: Field listings operations cannot be altered

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