@sgiehl opened this Pull Request on August 17th 2020 Member

After merging this we may need to update the DI definitions if various other plugins.
Once this is finished and approved I'll create PRs accordingly...

fixes #15974

@sgiehl commented on August 17th 2020 Member

@tsteur Not sure if updating the lib might cause problems while updating. The method DI\object has been removed, which is in use by some plugin configs. So it may cause fatals unless the plugin is updated together with core.

Also did not yet find a easy way to solve this:

there are some issues with having references in functions defined in configuration like here https://github.com/matomo-org/matomo/blob/0187b71729484a9f173d978239ff0a3cb37a5d7b/config/environment/test.php#L101

Might be required to change all event definitions to not use references if there is no other solution.... Will have a closer look at that...

@tsteur commented on August 17th 2020 Member

@sgiehl is there any way that we define \Di\object ourselves maybe so it won't break? I just had a look it be mostly only PaidAds and SearchEngine plugin if I'm seeing this right? The other ones be mostly cloud related plugins or core plugins.

@sgiehl commented on August 18th 2020 Member

guess we could try using autowire as alias, might work in the most cases...

@sgiehl commented on August 18th 2020 Member

Wrapping the observer functions into \DI\value() seems to fix the problem. Not sure if that has any disadvantages 🤷
At least the tests seem to pass again.

I've also added a proxy for \DI\object() to \DI\autowire(), which should circumvent any possible failures due to plugins still using \DI\object()

Should be ready for a first review...

@tsteur commented on August 27th 2020 Member

@sgiehl can you adjust the docs in https://developer.matomo.org/guides/dependency-injection ? It's showing DI\object a few times and it be great to describe when to use autowire and when create. Not sure if it's also mentioned in other places (likely not).

@tsteur commented on August 27th 2020 Member

The docs mention also DI\link which seems now removed? Need to update this in our docs and changelog

@tsteur commented on August 27th 2020 Member
@tsteur commented on August 27th 2020 Member

Generally looks good otherwise. Be great to clarify usage of create vs autowire, update the docs etc. And be great if you could then also update the other free plugins and premium features after merging

@tsteur commented on August 27th 2020 Member

I suppose we might also need to create new releases for plugins that already had a 4.x release

@sgiehl commented on August 27th 2020 Member

If I understood that correctly: DI\autowire needs to be used if any constructor parameter needs to be set by DI. If the definition already defines all parameters or none is needed you can use DI\create. So actually I guess it should be safe to always use autowire maybe 🤷

@sgiehl commented on August 27th 2020 Member

I suppose we might also need to create new releases for plugins that already had a 4.x release

If their DI config need to be updated... For some plugins it's only test config, so a new release might not be needed...

@tsteur commented on August 27th 2020 Member

reading eg https://stackoverflow.com/questions/49474983/how-to-use-autowire-create-and-get-in-definitions-for-php-di I reckon autowire is a good choice that should always work.

@tsteur commented on August 27th 2020 Member

looks otherwise good to merge. Need to update docs though.

@sgiehl commented on August 31st 2020 Member

I've also added a proxy from DI\link to DI\get now, so plugins that could possibly use DI\link won't break on update...

@tsteur commented on August 31st 2020 Member

Sweet. Feel free to merge @sgiehl . Looks like some merge conflicts will need to be resolved.

This Pull Request was closed on September 3rd 2020
Powered by GitHub Issue Mirror