https://github.com/matomo-org/matomo/pull/16311 - Mouve

Site d'origine

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updates php-di to 6.2.1 #16311

Merged
merged 15 commits into from Sep 3, 2020
Merged

Updates php-di to 6.2.1 #16311

merged 15 commits into from Sep 3, 2020

Conversation

@sgiehl
Copy link
Member

@sgiehl sgiehl commented Aug 17, 2020

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 sgiehl added this to the 4.0.0 milestone Aug 17, 2020
@Findus23 Findus23 mentioned this pull request Aug 17, 2020
6 of 6 tasks complete
@sgiehl sgiehl force-pushed the phpdi branch from d372ebf to 87e4847 Aug 17, 2020
@sgiehl
Copy link
Member Author

@sgiehl sgiehl commented Aug 17, 2020

@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

array('AssetManager.getStylesheetFiles', function (&$stylesheets) {

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
Copy link
Member

@tsteur tsteur commented Aug 17, 2020

@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
Copy link
Member Author

@sgiehl sgiehl commented Aug 18, 2020

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

@sgiehl sgiehl force-pushed the phpdi branch 3 times, most recently from 169d706 to 7c5ee00 Aug 18, 2020
@sgiehl
Copy link
Member Author

@sgiehl sgiehl commented Aug 18, 2020

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...

@sgiehl sgiehl marked this pull request as ready for review Aug 18, 2020
@sgiehl sgiehl force-pushed the phpdi branch from 7c5ee00 to 5df6070 Aug 18, 2020
@tsteur
Copy link
Member

@tsteur tsteur commented Aug 27, 2020

@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).

CHANGELOG.md Outdated Show resolved Hide resolved
@tsteur
Copy link
Member

@tsteur tsteur commented Aug 27, 2020

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

@tsteur
Copy link
Member

@tsteur tsteur commented Aug 27, 2020

CHANGELOG.md Outdated Show resolved Hide resolved
@tsteur
Copy link
Member

@tsteur tsteur commented Aug 27, 2020

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
Copy link
Member

@tsteur tsteur commented Aug 27, 2020

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

@sgiehl
Copy link
Member Author

@sgiehl sgiehl commented Aug 27, 2020

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
Copy link
Member Author

@sgiehl sgiehl commented Aug 27, 2020

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...

sgiehl added 5 commits May 18, 2020
…o undefined method
sgiehl added 2 commits May 18, 2020
@sgiehl sgiehl force-pushed the phpdi branch from 5df6070 to 0730c9f Aug 27, 2020
@tsteur
Copy link
Member

@tsteur tsteur commented Aug 27, 2020

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
Copy link
Member

@tsteur tsteur commented Aug 27, 2020

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

@sgiehl
Copy link
Member Author

@sgiehl sgiehl commented Aug 31, 2020

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
Copy link
Member

@tsteur tsteur commented Aug 31, 2020

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

diosmosis and others added 3 commits Sep 2, 2020
tsteur added a commit to matomo-org/tag-manager that referenced this pull request Sep 3, 2020
tsteur added a commit to matomo-org/plugin-MarketingCampaignsReporting that referenced this pull request Sep 3, 2020
tsteur added 2 commits Sep 3, 2020
@tsteur tsteur merged commit 3565da7 into 4.x-dev Sep 3, 2020
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
@tsteur tsteur deleted the phpdi branch Sep 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants

level de log mis à 1

Récupération du document https://github.com/matomo-org/matomo/pull/16311.

Récupération de l'URL https://github.com/matomo-org/matomo/pull/16311...

Raccourcis

Commandes

Fermer