Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove cca dependency and implement compatibility for Symfony ^4.4 || ^5.1 #111

Merged
merged 3 commits into from
Aug 23, 2021
Merged

Remove cca dependency and implement compatibility for Symfony ^4.4 || ^5.1 #111

merged 3 commits into from
Aug 23, 2021

Conversation

fritzmg
Copy link
Contributor

@fritzmg fritzmg commented Aug 18, 2021

Fixes #107

This also introduces proper Symfony dependencies and implements full compatibility with Symfony 4 and 5.

stefanheimes and others added 3 commits May 10, 2021 00:55
@stefanheimes
Merge tag '3.4.11'
298df57 
Hotfix release 3.4.11

- Replace all key_exists with array_key_exists
- Solve a problem with Contao 4.11
- Fix for the column field with hiden header
- Add missing authors
- Clean php code
@fritzmg
remove the CCA dependency and add appropriate symfony dependencies in…
26b52a7 
…stead
@fritzmg
Symfony >=4.4 compatibility
b7ffc3a
@fritzmg fritzmg mentioned this pull request Aug 18, 2021
Closed
@zonky2 zonky2 added this to the 4.0.0 milestone Aug 22, 2021
@zonky2 zonky2 requested a review from stefanheimes August 23, 2021 13:44
@zonky2 zonky2 added the enhancement New feature or request label Aug 23, 2021
@zonky2
Copy link
Contributor

zonky2 commented Aug 23, 2021

@stefanheimes took a look at it - the MCW must also remain usable for Contao 4.4 and therefore the PR cannot be adopted as it is.

However, the general problem is being worked on.

@fritzmg
Copy link
Contributor Author

fritzmg commented Aug 23, 2021 •
edited
Loading

@stefanheimes took a look at it - the MCW must also remain usable for Contao 4.4 and therefore the PR cannot be adopted as it is.

This PR has no effect on it being usable in Contao 4.4. After merging this PR, a new minor version should be released. You can then still provide bugfixes for the previous minor version, if you ever need to (which is unlikely anyway, as Contao 4.4 is outdated anyway and only gets security fixes).

@fritzmg
Copy link
Contributor Author

fritzmg commented Aug 23, 2021

zonky2 added this to the 4.0.0 milestone yesterday

This must not be released as a new major version. If you do, it would not resolve the original issue.

@zonky2
Copy link
Contributor

zonky2 commented Aug 23, 2021

This must not be released as a new major version. If you do, it would not resolve the original issue.

I only do the "secretary job" - that's up to @stefanheimes

@fritzmg
Copy link
Contributor Author

fritzmg commented Aug 23, 2021

Then may be @stefanheimes should join this discussion here 😁

@stefanheimes
Copy link
Member

Question @fritzmg: Why adding symfony/http-foundation or symfony/http-kernel as requirement. We didn't use them. So why adding more blocker to the list of requirements than needed? Why should I add all the sub requirements to the list, I'm only interested in the direct requirements and the result from them and not for the second or third sublevel requirement in the list.

@fritzmg
Copy link
Contributor Author

fritzmg commented Aug 23, 2021 •
edited
Loading

Why adding symfony/http-foundation or symfony/http-kernel as requirement. We didn't use them.

You are using these components here:

So why adding more blocker to the list of requirements than needed? Why should I add all the sub requirements to the list, I'm only interested in the direct requirements and the result from them and not for the second or third sublevel requirement in the list.

These are direct dependencies. Sure, both the http-foundation and http-kernel will likely always already be there, since they are used by Contao. But the same assumption was made for sensio/framework-extra-bundle for example and then when Contao did not need that package anymore, extensions broke because they did not properly require their dependencies.

Furthermore extensions broke because they did not properly require symfony/dependency-injection in the correct version.

And similarly extensions broke because they did not properly require symfony/config in the correct version.

Especially the latter is a problem that is still present in many extensions today. So imho either you properly require all your actual dependencies, or you live with the fact that you might have to hotfix something, once one of these dependencies change in an incompatible way in newer versions (or if it gets dropped altogether).

Also the fact that menatwork/contao-multicolumnwizard-bundle was missing the dependency for symfony/http-kernel in the appropriate version is what was causing the problem in https://github.com/menatwork/contao-multicolumnwizard-bundle/issues/107 in the first place. No issue would have occurred, if for example symfony/http-kernel: ^3.0 || ^4.0 was defined as a dependency. Yes, it turned out later that the troublesome code was not necessary anyway and was subsequently removed in 1e0db7c - but that's beside the point.

And these dependencies do not really block anything for Contao 4, as the switch to Symfony 6 will not happen for Contao 4.

@stefanheimes
Copy link
Member

@fritzmg Thanks for the help. My problem was I had used the package name instead of the right Namespace for the search. So I totally missed this things. You are right it would be much better to add the requirements, so that we always have what we need.

All looks okay, I think I will make a new 3.5. There are no API breaks, only the argument change for the events dispatcher :P

@stefanheimes stefanheimes changed the base branch from master to develop August 23, 2021 21:46
@fritzmg
Copy link
Contributor Author

fritzmg commented Aug 23, 2021 •
edited
Loading

There are no API breaks, only the argument change for the events dispatcher :P

Yes, this was changed in Symfony 4, with BC to the old Symfony 3 way. In order to be compatible with Symfony 4 and 5 though, only the new way works (event object as first argument, optional event name as second).

But this does not break the MCW API.

@stefanheimes stefanheimes merged commit 22b8513 into contao-community-alliance:develop Aug 23, 2021
@stefanheimes
Copy link
Member

@fritzmg I have merged this PR into dev, which is the pre version for the 3.5.0. Can you test if everthing is working for you ?

@stefanheimes stefanheimes modified the milestones: 4.0.0, 3.5.x Aug 23, 2021
@fritzmg fritzmg deleted the remove-cca-dependency branch August 24, 2021 07:38
@fritzmg
Copy link
Contributor Author

fritzmg commented Aug 24, 2021

Everything seems to work fine as far as I tested 👍

@fritzmg fritzmg mentioned this pull request Aug 24, 2021
Closed
@fritzmg
Copy link
Contributor Author

fritzmg commented Aug 27, 2021 •
edited
Loading

@zonky2 @stefanheimes any chance to get this released as 3.5.0 soon? The problem keeps piling up on the forum and in Slack ;)

@zonky2
Copy link
Contributor

zonky2 commented Aug 27, 2021

its @stefanheimes turn...

@stefanheimes
Copy link
Member

stefanheimes commented Aug 29, 2021 •
edited
Loading

I have added a new support/contao4.4 branch.
I merged all changes from dev into master with the new version 3.5.0.
I replaced all the branch alias entries with the right ones.

@fritzmg @zonky2 should be fine now :)

@stefanheimes
Copy link
Member

@fritzmg by the way, do you see any problem with this pr #112 if I merge this in ther current 3.5.0 branch ?

@fritzmg
Copy link
Contributor Author

fritzmg commented Aug 29, 2021

@fritzmg by the way, do you see any problem with this pr #112 if I merge this in ther current 3.5.0 branch ?

Should be fine, but it was not merged correctly: https://github.com/menatwork/contao-multicolumnwizard-bundle/pull/114

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stefanheimes stefanheimes Awaiting requested review from stefanheimes

Assignees

@stefanheimes stefanheimes

@fritzmg fritzmg

Labels
enhancement New feature or request
Projects
None yet
Milestone
MAW/CCA 3.5.x
Development

Successfully merging this pull request may close these issues.

Problem mit Contao 4.11.1 und Parameter "kernel.root_dir"
3 participants
@fritzmg @zonky2 @stefanheimes