Updated: Comment #72
Problem/Motivation
Typed data plugins need to have dependencies injected. See #2903549: Replace REQUEST_TIME with time service in CreatedItem as an example of plugin that can benefit from this
Proposed resolution
1. Make TypedDataManager container aware (use ContainerAwareTrait)
2a. Check if created plugin implements ContainerAwareInterface, and if yes, call its setContainer() method.
2b. Check if plugin definition contains "services" section. If no - same logic as before. If yes,
3a. Inject services in additional parameter of createInstance()
3b. Assume that plugin implements ContainerFactoryPluginInterface, call ContainerFactoryPluginInterface::create()
3c. Directly call plugin constructor
2a is 2a and 3b are fully backwards compatible.
3a should be BC as well (on assumption that no plugins use extra parameters to createInstance())
Remaining tasks
Decide what is the best approach
Write the patch
Test it
Review it
User interface changes
None
API changes
Add @service_container as a parameter of typed_data_manager service
Related Issues
#1951268: Convert /forum and /forum/% to new router, remove forum_forum_load(), forum_get_topics(), create Forum service
#2914419: Allow Plugins to specify Services via Annotation
Comment | File | Size | Author |
---|---|---|---|
#71 | 2053415-71.patch | 1.94 KB | valthebald |
#71 | interdiff-2053415-68-71.txt | 4.02 KB | valthebald |
#68 | 2053415-68.patch | 4.63 KB | valthebald |
#68 | interdiff-2053415-66-68.txt | 1.16 KB | valthebald |
#66 | 2053415-66.patch | 4.6 KB | valthebald |
Comments
Comment #1
larowlanbefore @timplunkett grabs it ;)
Comment #2
larowlanLets see
Comment #3
tim.plunkettThere is no reason to have
= NULL
for any of theseMight as well wrap these onto multiple layers.
Good idea!
Comment #5
dawehnerNeeds doc changes.
Comment #6
BerdirI think you picked a somewhat bad example because I think that this class could now get the information it needs from the $field/fieldItem class and doesn't need the field info service. $field->getFieldDefinition()->getSetting(). That would also make it re-usable without a configurable field (or at least a step into that direction).
Comment #7
larowlanDamn, I spent nearly 30 mins looking for an example.
I need it for forum container, but wanted this in separate.
Comment #8
larowlan\Drupal\Core\TypedData\Plugin\DataType\Map is a good candidate as it uses \Drupal::typedData()
Comment #9
BerdirThe TypedData base class itself uses the typed data manager too, for constraint/validation stuff for example.
Comment #10
larowlanThis does TypedData and Map and at least installs (which is a start).
Lets see what else is broken.
Comment #12
larowlanThis one installs locally (For real this time).
Comment #14
larowlanso TypedConfigManager and LocaleTypedConfig need some of the injection action too...
I wish api.d.o was up to date for the inheritance map.
And I mixed a property with a method.
I think that quantity of fails and exceptions is a personal record.
Comment #16
larowlanException: Serialization of 'Symfony\Component\HttpFoundation\File\UploadedFile' is not allowed in serialize() (line 99 of /core/lib/Drupal/Core/KeyValueStore/DatabaseStorageExpirable.php)
Going to need some pointers/help on finding/fixing that.
Comment #17
larowlanTypedData->typedDataManager->languageManager->request->files was the culprit.
We've got some serious serializing going on here, I think this needs profiling.
Comment #19
larowlanNodeTest was new, but EntityTranslation was valid, request object on LanguageManager is optional
Comment #20
dawehnerNearly RTBC.
You could just use {@inheritdoc}
Comment #21
larowlanThe TypedDataInterface param isn't in the parent, hence I didn't use inheritdoc
Comment #22
Berdir$node = node_load(5);
dpm(strlen(serialize($node)));
HEAD:
8401
With this patch:
81032
That's not cool :)
Let's just drop and re-fetch the typed data manager in serialize()/unserialize of typed data objects.
Later on, I want to investigate if we can to something like this in EntityNG or on the typed data level:
Thats 5553 for my node. That however isn't going to be trivial and needs thorough profiling in cpu vs. memory, especially the unserialize part, so this is something to figure out later...
Comment #23
larowlanthanks will do.
berdir++
Comment #24
larowlantackling
Comment #25
larowlanWe've handled the serialization issue so removing the tag.
Fixes #22
Comment #26
larowlanComment #28
larowlaninfinite recursion when serializing basically.
so we'll probably need #22 sooner, giving it a try locally
Comment #29
BerdirMaybe TypedData manager instead of typedDataManager?
This will trigger __get() :(
Mabe use __sleep()/__wakeup() instead?
Comment #30
larowlanPerhaps this?
Comment #32
BerdirThis might be easier if we wait for #2004282: Injected dependencies and serialization hell then we can use the same pattern..
Comment #33
smiletrl CreditAttribution: smiletrl commentedI come from #2083937-8: Make typed data class dependency injectable., so here're some changes related.
1).
TypedData extends DependencySerialization, instead of implementing \Serializable to deal with serialization.
2).
Because typedData class may have __get() and __set(), rewrite __sleep() of DependencySerialization to avoid possible indirect modification of overloaded property.
3).
class_implements($class, '\Drupal\Core\Plugin\ContainerFactoryPluginInterface')
will ALWAYS return true, as long as $class has implemented interface(s). So4).
Not all typed data need to inject services, so set the last paramter $typedDataManager to be optional for abstract class TypedData.
For example, LocaleTypedConfig doesn't need the injection at all. So remove all unnecessary changes associated with this class, which changes are simply to be consistent with the inject pattern, but doesn't make much sense for the actual usage.
Two questions:
1).
ContainerFactoryPluginInterface is not a perfect interface for our usage here. Its
::create
includes an extra paramterarray $plugin_definition
which is never be used here. But we have to add this parameter for typed Data's::create method
and add this redundant pamater every time when this::create
method is called.Is it worth to create a new TypedDataInjectionInterface to avoid this redundant parameter, i.e.,
array $plugin_definition
from ContainerFactoryPluginInterface, and match the exact TypedData's create pattern?2).
Current code has added the type hint
TypedDataManager $typed_data_manager = NULL
in typed Data's construct method.Therefore, we need to add following line too:
I'm wondering is it really necessary to add the type hint here. The inject pattern allows developers to inject pseudo/different services/objects into class in unit test or even contrib code. If it has limited object $typed_data_manager to be instance of calss "Drupal\Core\TypedData\TypedDataManager", then what's the point of this dependency inject pattern? I guess we should remove the type hint here imho.
Comment #35
smiletrl CreditAttribution: smiletrl commentedRerolled.
Also, TextProcessed doesn't need injection too. So, remove unnecessary changes.
Comment #37
smiletrl CreditAttribution: smiletrl commentedFix some bugs
Comment #39
smiletrl CreditAttribution: smiletrl commentedThis problem happens to "Drupal\Core\Entity\Field\Field".
I feels it's kind of related with TypedDataManager::getPropertyInstance, and more specificly, it's about the prototyping. During the prototype process,
dynamic property _serviceIds (which was created in __sleep() phase) has been overridden.Porperty typedDataManager, which was created in __construct phase, was overridden.For someone interested, here's how to reproduce the problem:
Or, you may try:
Comment #39.0
smiletrl CreditAttribution: smiletrl commentedUpdated issue summary.
Comment #40
XanoRe-roll.
Comment #42
dixon_Re-roll with some manual updates. Things apparently changed a lot since 6 months ago ;)
I'm not sure why this was introduced here. Doesn't seem to be used anywhere. Removed in attached patch.
$configuration
seems to be the wrong argument here. I believe we need the data definition instead. Fixed in the attached patch.Comment #44
dixon_Never mind regarding #42.2, it's of course the
ContainerFactoryPluginInterface
dictating that...I'm still getting some install errors, but uploading patch for others to take a look at.
Comment #46
BerdirYeah, but ContainerFactoryPluginInterface doesn't work for us, we're not using the default factory either as our constructor is different, and create() has to match __construct().
Needs a typed data specific interface I guess?
Comment #47
Jose Reyero CreditAttribution: Jose Reyero commentedHey, about fixing the TypedConfigManager, see #2277945: Typed configuration implements TypedDataInterface improperly
I think that should help this patch. Also this patch will help fixing a number of issues with typed config, like validation, etc, and overall it will help adding some more consistency.
Comment #48
Jose Reyero CreditAttribution: Jose Reyero commentedRelated #2281635: \Drupal\Core\Plugin\Context\Context should use a ContextDefinition class instead of arrays
See new \Drupal\Core\TypedData\TypedDataTrait
Comment #49
BerdirThat has nothig to do with this, it only allows to inject the typed data manager into something, not inject other services into typed data plugins.
Comment #50
Jose Reyero CreditAttribution: Jose Reyero commented@Berdir,
Maybe not, you're right, I've re-read the issue and it's not exactly what I thought it was, but..
...anyway, this is how I see it, let me know whether this makes sense or not:
We happen to have typed configuration and typed configuration manager reusing all this stuff. The whole point of TypedConfigManager is being able to use all typed data plugins for typed configuration. And with the changes proposed here that will be still harder.
Which other services do we need to be injected? Because the only one I see plugins using is typed data manager, and that is the one that differs with typed config and we need to be able to inject/change it.
I don't think that works if we want to be able to use these plugins with a different 'typed data manager' which is the case for configuration. We need to pass the typed data manager as a parameter for that.
On the other side having two simple methoget/setTypedDataManager() would fix it for both TypedDataManager and TypedConfigManager.
So please let's try for typed data to be reusable and not making it still more difficult.
Comment #51
larowlanshameless bump
Comment #52
XanoEither that, or we need to expand the interface so we can inject the extra data. Abusing the configuration array for this (which is plugin-specific and not plugin-type-specific) is a bad idea.
On the one hand introducing a new factory interface isn't very pretty, but adding setters on the plugin type interface can give people the false idea that those values might change during a plugin instance's lifetime.
Comment #53
bojanz CreditAttribution: bojanz at Centarro commentedIs there still a chance to move forward with this?
TypedData not supporting dependency injection is one of its most visible ugly sides.
Comment #54
wizonesolutionsI'm guessing this has to be part of 8.1.x now?
Given the changes to Drupal 8, what would be the best way to reroll this? From what I'm reading, we want to avoid using
ContainerFactoryPluginInterface
...but at the same time, we're not sure if we want to make another injection interface just forTypedData
.I came here because I saw
FileFieldItemList
using the globalDrupal
container to call services, and that didn't feel right since it's not procedural. A couple tweets later, and I was pointed to this issue.Comment #57
ohthehugemanatee CreditAttribution: ohthehugemanatee at Amazee Labs commentedI'm here because I saw EntityReference calling the Entity Storage service with the global container. This issue seems pretty dead though... What's the next step here?
Comment #59
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedIt looks like some parts of this patch are outdated perhaps? Overall it should not be that hard, but need to watch out for side effects.
Seems a bit tricky since \Drupal\Core\TypedData\TypedData::createInstance is a static method
It might make sense to use setter injection here so the main constructor is unchanged?
Comment #61
valthebaldComment #62
joachim CreditAttribution: joachim as a volunteer commentedShouldn't this be the same pattern as plugins, where the factory checks whether the plugin implements ContainerFactoryPluginInterface?
Comment #63
BerdirNo because other plugins have different arguments, that doesn't work here.
Comment #64
mradcliffeWould it be better to add this as the last argument instead of the first to match the pattern of the other plugin managers?
Maybe we need a new injection interface for type data plugins to implement that defines a new static method (something like createInjectableInstance?), and then typed data manager can check if the class implements that interface and passes the container into it. That probably is best as the first argument to match the other injection patterns.
Comment #66
valthebaldFix the failing test (it fails because TypedDataManager has additional constructor parameter), and make container the last parameter per #64.
Regarding new interface... not sure about this proposal. This will require checking the field type, and the only difference is having container in (ignored by non-DI fields) parameter. I think single (existing) interface is better
Comment #67
ndobromirov CreditAttribution: ndobromirov at FFW commentedThis is adding an API change on the create method for all plugins, so it will cause contrib code to break.
Comment #68
valthebald@mradcliffe: actually, we don't need new interface, but plugins that need container can implement ContainerAwareInterface (and related trait). Here's the patch:
Comment #69
mradcliffeA new interface with a different method name would be backwards-compatible (see @Berdir's comment in #63). I don't like the idea of a new interface, but the create and createInstance methods already exist on the TypedData class, and that makes a big API change in my opinion.
Comment #70
valthebaldHaving container aware plugins is also backward compatible, no?
Comment #71
valthebaldSimpler version. Inject container into TypedDataManager via "calls". This should work with no test modification.
Data plugins that need injection should implement ContainerAwareInterface
Comment #72
mradcliffeAh, yes, it would be for TypedData classes.
I think that ContainerInjectionInterface::create would be the pattern that other plugin systems use though and not ConatinerAwareInterface.
Comment #73
valthebaldContainerInjectionInterface::create() does not fit, since it has only 1 parameter. The closest by parameters (and logically) is ContainerFactoryPluginInterface::create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition);
Question is how to determine if plugin needs injection or not? Possible way it to add services list to plugin annotation:
Then, in TypedDataManager::createInstance():
$services then can be appended to $class::createInstance() parameters - either in a single array parameter, or every service appended as a separate parameter. I'd prefer every service being separate parameter, as it won't affect "container unaware" (read "all existing") typed data plugins
Opinions?
Comment #74
mradcliffeThat's why I suggest a new interface. An interface solves whether or not to inject the container or not because we can detect whether a class implements an interface. If it does, call the static. If not, call the constructor with the default arguments.
Comment #75
valthebald1. Do you suggest the new interface only for plugins that needs injection? If yes, how do you distinguish between "injectable" and "regular" plugins?
2. Why injectable plugins cannot implement ContainerFactoryPluginInterface?
Comment #76
valthebaldThe idea of using plugin annotation to describe what services it needs, is suggested also in #2914419: Allow Plugins to specify Services via Annotation
Comment #77
valthebaldComment #78
mradcliffeChatted with valthebald in Slack and he pointed me to a similar effort in #2914419: Allow Plugins to specify Services via Annotation. If that pattern is OK, then I think Typed Data annotations can do similarly though we cannot use the ServiceFactory introduced there because of the argument differences. Unless ServiceFactory was changed to also send in default arguments from the plugin manager.
I think it should be something like the following rather than injecting the container into the typed data object if we need to implement this on our own without ServiceFactory.
Comment #79
valthebaldUpdated issue summary to list what are possible options. Time to decide :)
Comment #80
valthebaldComment #81
ndobromirov CreditAttribution: ndobromirov at FFW commentedI would block this until https://www.drupal.org/project/drupal/issues/2914419 is resolved, as it's essentially the same issue and 2 core plugin maintainers are on it.
Once there is an agreed way of how to do this in core this issue will be trivial.
Comment #90
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commented#2914419: Allow Plugins to specify Services via Annotation seems totally stalled. Maybe we should revive this simpler fix?
Comment #91
BerdirThe last patches here aren't DI though, it's just making them containerAware. Yeah, the factory pattern isn't great either but this seems even worse IMHO.
Comment #93
dmitry.korhovSo what is your proposal for this issue? I do not think we can leave it as is for next 5-9 years :)
I would suggest to complete proposed solution with passing containerInterface into typedData as we had it in a lot of other plugins.
Strange to see that ItemList (which uses TypedData) still has no proper DI for years and uses trait with dependencies from global scope
Comment #96
bradjones1I'm going to mark this a duplicate of #3294266: Allow plugin service wiring via constructor parameter attributes which seems to target the underlying issue more broadly for plugin types of all kinds.