Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
As per #2047983: Mark field_info_*_types() / field_info_*_settings() functions as deprecated. - those should use the getDefinition(s) methods on the plugin managers
Comment | File | Size | Author |
---|---|---|---|
#27 | remove_field_info_types_settings-2047993-27.patch | 651 bytes | plopesc |
#23 | remove_field_info_types_settings-2047993-23.patch | 52.83 KB | plopesc |
#23 | interdiff_21-23.txt | 2.09 KB | plopesc |
#21 | remove_field_info_types_settings-2047993-21.patch | 51.08 KB | plopesc |
#16 | remove_field_info_types_settings-2047993-16.patch | 53.12 KB | plopesc |
Comments
Comment #1
plopescWorking on this one.
Regards
Comment #2
plopescFirst 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.
Comment #4
tstoeckler@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.
Comment #5
plopescRe-rolling patch removing deprecated API functions. Running testbot again.
Comment #6
yched CreditAttribution: yched commentedThanks @plopesc!
Yes, let's leave the functions around for now, we'll check committers feedback to see if the removal is acceptable.
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
No need to re-pull the FormatterPluginManager from itself, it's $this ;-) (hem, which shows how silly the current code is...)
Same in WidgetPluginManager.
Comment #7
yched CreditAttribution: yched commented@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 :-)
Comment #8
yched CreditAttribution: yched commentedOops, #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.
Comment #9
plopesc@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!
Comment #10
yched CreditAttribution: yched commentedHah, right, I skipped that. Must've been added while I was away for a while. Sweet :-)
Comment #11
plopescHello
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
Comment #12
yched CreditAttribution: yched commentedThanks !
(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.
(phpdoc for getDefaultInstanceSettings())
"... the default instance-level settings for a field type"
(phpdoc for getDefaultInstanceSettings())
"A field type name"
Minor: at this point, better extract to a variable to have shorter lines
Comment #13
plopescNew 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.
Comment #14
yched CreditAttribution: yched commentedLooks good, thanks !
Only minor phrasing nitpicks:
Then for consistency with the comment above, it should be "The manager for formatter plugins".
"The 'field type' plugin manager".
same
same
Comment #16
plopescSorry
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.
Comment #17
yched CreditAttribution: yched commentedI 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.
Comment #18
tim.plunkett$this->container is preferred in tests, but it makes little difference.
Comment #19
yched CreditAttribution: yched commentedOK, green and @tim.plunkett's #18: RTBC then.
Thanks !
Comment #20
alexpottNeeds a reroll...
Comment #21
plopescRerolling...
Comment #22
yched CreditAttribution: yched commentedYeah, 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:
#21 doesn't seem to do this, so I don't think it will work.
Comment #23
plopescSorry, forgot to review this class :(
Injecting FieldTypeManager in FieldInstanceEditForm now.
Comment #24
yched CreditAttribution: yched commentedThanks!
Looks good if comes back green.
Comment #25
catchCommitted/pushed to 8.x, thanks!
Comment #26
YesCT CreditAttribution: YesCT commentedin #2045043-17: Field listings operations cannot be altered we did
parent::__construct($entity_manager);
kinda cool.
Comment #27
plopescAttaching change suggested by YesCT.
Regards.
Comment #28
yched CreditAttribution: yched commentedNo need, #2045043: Field listings operations cannot be altered is taking care of it :-)
Comment #29
yched CreditAttribution: yched commentedtag slipped in.
Comment #30
rahul.metallica CreditAttribution: rahul.metallica commented#2: remove_field_info_types_settings-2047993-2.patch queued for re-testing.
Comment #31
yched CreditAttribution: yched commentedNonono, 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