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

Do not require symfony/symfony #17

Merged
merged 3 commits into from Aug 21, 2018
Merged

Do not require symfony/symfony #17

merged 3 commits into from Aug 21, 2018

Conversation

leofeyer
Copy link
Member

Currently Contao 4.6 implicitly requires at least Symfony 4.1, because we need symfony/http-kernel and symfony/monolog-bridge in at least version 4.1.

However, if we stop requiring symfony/symfony, all components but the two mentioned above could be installed in version 3.4, 4.0 or 4.1, therefore this PR replaces requirements for symfony/symfony and for symfony/framework-bundle with their respective components.

@contao/developers Do you prefer merging this PR so Contao 4.6 is compatible with Symfony 3.4 or do you prefer to keep symfony/symfony with Contao 4.6 not being compatible with Symfony <4.1 anymore?

@leofeyer leofeyer added the bug label Aug 21, 2018
@leofeyer leofeyer added this to the 4.6.0 milestone Aug 21, 2018
@leofeyer leofeyer self-assigned this Aug 21, 2018
@bytehead
Copy link
Member

I'm definitely for merging this! 👍

@Toflar
Copy link
Member

Toflar commented Aug 21, 2018

because we need symfony/http-kernel and symfony/monolog-bridge in at least version 4.1.

Just for some background info: why is this?

@leofeyer
Copy link
Member Author

To fix the logging:

contao/manager-bundle/src/Resources/skeleton/app/config_prod.yml

Line 14 in ccab809

excluded_http_codes: [400, 401, 403, 404]

The feature has been added in version 4.1.

@Toflar
Copy link
Member

Toflar commented Aug 21, 2018

Okay, any reason why we do not allow 4.2?

@leofeyer
Copy link
Member Author

leofeyer commented Aug 21, 2018 •
edited

Because it has not yet been released. 😄

@Toflar
Copy link
Member

Toflar commented Aug 21, 2018

I'm fine with the changes then, thanks for the additional info!

@leofeyer leofeyer force-pushed the no-more-symfony-symfony branch 3 times, most recently from ab91c2d to cee8f06 Compare August 21, 2018 08:06
@aschempp
Copy link
Member

I don't think it makes sense to allow some components in 4.1 and some in 3.4. That will only cause issues in the long run. The only reason to allow 3.4 would be so someone that really requires 3.4 can still update, but that possibly won't work because some of the components are not compatible …

Honestly I'm usually in favor of keeping compatibility, but if we raise the dependency to 4.1 then we can also get rid of some stupid stuff like our SessionListener that needs to work across multiple incompatible versions…

@aschempp
Copy link
Member

Also, allowing a constraint of symfony/http-kernel 3.4 for all dependencies only makes the solver slow because it can never be installed when all bundles have a requirement on contao/core-bundle…

@leofeyer leofeyer merged commit 58ad40c into 4.6 Aug 21, 2018
@leofeyer leofeyer deleted the no-more-symfony-symfony branch August 21, 2018 13:58
@fritzmg
Copy link
Contributor

fritzmg commented Aug 24, 2018 •
edited

Currently a lot of bundle extensions aren't compatible with Contao 4.6, since their DCA callback and Hook listener services are not defined as "public". As a workaround, one could just require symfony/dependency-injection:^3.0 - however that does not work in Contao 4.6.0 due to the dependency in the core-bundle on symfony/http-kernel:^4.1 which in turn requires symfony/dependency-injection:^4.1. This means I have to wait until all my dependencies have been updated for Contao 4.6 (wherever applicable), before I can use it in production.

@ghost ghost mentioned this pull request Aug 25, 2018
Closed
@ghost
Copy link

ghost commented Aug 25, 2018

A new issue thereto has been created at #30.

@richardhj richardhj mentioned this pull request Aug 27, 2018
Merged
leofeyer pushed a commit that referenced this pull request Aug 4, 2020
@qzminski
Fix a potential error if the URL has percentage in it (see #619)
0a7b13d 
Description
-----------

With @Toflar we have discovered by accident that if a URL is double-encoded (for some reason, doesn't matter) the Contao's `RouteProvider` will eventually throw an error trying to query a database.

```
URL original: drachenlochmuseum-v%25c3%25a4ttis.html
URL decoded: drachenlochmuseum-v%c3%a4ttis.html
URL decoded 2nd time: drachenlochmuseum-vättis.html
```

The decoded URL is used in the database query and that fails because the database driver would like to replace wildcards `%c` with parameters that were not provided.

Stack trace:

```
Exception: Too few arguments to build the query string
#27 vendor/contao/core-bundle/src/Resources/contao/library/Contao/Database/Statement.php(304): replaceWildcards
#26 vendor/contao/core-bundle/src/Resources/contao/library/Contao/Database/Statement.php(249): execute
#25 vendor/contao/core-bundle/src/Resources/contao/library/Contao/Model.php(1102): find
#24 vendor/contao/core-bundle/src/Resources/contao/library/Contao/Model.php(973): findBy
#23 vendor/contao/core-bundle/src/Framework/Adapter.php(38): __call
#22 vendor/contao/core-bundle/src/Routing/RouteProvider.php(493): findPages
#21 vendor/contao/core-bundle/src/Routing/RouteProvider.php(88): getRouteCollectionForRequest
#20 vendor/contao/core-bundle/src/Routing/LegacyRouteProvider.php(43): getRouteCollectionForRequest
#19 vendor/symfony-cmf/routing/src/NestedMatcher/NestedMatcher.php(141): matchRequest
#18 vendor/contao/core-bundle/src/Routing/Matcher/LegacyMatcher.php(69): matchRequest
#17 vendor/symfony-cmf/routing/src/DynamicRouter.php(271): matchRequest
#16 vendor/symfony-cmf/routing/src/ChainRouter.php(188): doMatch
#15 vendor/symfony-cmf/routing/src/ChainRouter.php(158): matchRequest
#14 vendor/symfony/http-kernel/EventListener/RouterListener.php(115): onKernelRequest
#13 vendor/symfony/event-dispatcher/EventDispatcher.php(212): doDispatch
#12 vendor/symfony/event-dispatcher/EventDispatcher.php(44): dispatch
#11 vendor/symfony/http-kernel/HttpKernel.php(126): handleRaw
#10 vendor/symfony/http-kernel/HttpKernel.php(67): handle
#9 vendor/symfony/http-kernel/Kernel.php(198): handle
#8 vendor/symfony/http-kernel/HttpCache/SubRequestHandler.php(85): handle
#7 vendor/symfony/http-kernel/HttpCache/HttpCache.php(448): forward
#6 vendor/symfony/framework-bundle/HttpCache/HttpCache.php(57): forward
#5 vendor/symfony/http-kernel/HttpCache/HttpCache.php(420): fetch
#4 vendor/contao/manager-bundle/src/HttpKernel/ContaoCache.php(46): fetch
#3 vendor/symfony/http-kernel/HttpCache/HttpCache.php(317): lookup
#2 vendor/symfony/http-kernel/HttpCache/HttpCache.php(192): handle
#1 vendor/friendsofsymfony/http-cache/src/SymfonyCache/EventDispatchingHttpCache.php(98): handle
#0 web/app.php(58): null
```

Commits
-------

8ae2582 Fix a potential error if the URL has percentage in it
8caaf25 Fix unit tests
509f762 Correctly encode the page aliases
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@leofeyer leofeyer

Labels
bug
Projects
None yet
Milestone
4.6
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@leofeyer @bytehead @Toflar @aschempp @fritzmg