Updated to support Symfony 5.0 & Twig 3.0 #930
Conversation
composer.json
Outdated
| "symfony/framework-bundle": "^3.4|^4.1|^5.0", | ||
| "symfony/options-resolver": "^3.4|^4.1|^5.0", | ||
| "symfony/property-access": "^3.4|^4.1|^5.0", | ||
| "symfony/security": "^3.4|^4.1|^5.0", |
There was a problem hiding this comment.
symfony/security has no 5.0 version
https://github.com/symfony/security/releases
There was a problem hiding this comment.
Ah ... strange ... I must've misread it before.
Just looking into this a little further, the only components in the code seem to be the HttpKernel & Console ... did you want me to narrow this list down a little further, or just fix up the version reference for the moment?
|
Any thoughts on this @sspat ? ... I can adjust the version for that component, or otherwise just remove it entirely as I can't see any reference to the security component in the codebase. The only components in the code seem to be the HttpKernel & Console ... did you want me to remove the unused components from the composer file? |
|
I'm only commenting here because I tried updating one of my apps to use Twig 3.0 and it failed out because of this bundle (since the 1.1.4 stable release didn't declare its dependency on Twig, I ended up with errors compiling the container instead of a Composer conflict). The As for the actual Symfony dependency list, just on a cursory scan of use statements, I'd suggest these additions (these are directly used by this bundle, don't rely on them to come in as transient dependencies):
Another thing to be aware of is the removal of You should probably also update the Travis config to run a build with Symfony 4.4 and 5.0, the CI for this PR would fail on that Symfony version because of the translator interface change. |
…luded in other packages), and backward compatibility with translator interface. Also updated travis CI
| $this->authorizationChecker = $authorizationChecker; | ||
| $this->securityToken = $securityToken; | ||
|
|
||
| if (!($translator instanceof LegacyTranslatorInterface) && !($translator instanceof TranslatorInterface)) { |
There was a problem hiding this comment.
Are you sure this is required?
I suppose this should work with both TranslatorInterface.
No need to force people to use Contracts
There was a problem hiding this comment.
Yes, it makes sure that you're getting either the deprecated interface from the component (available in Symfony 4.4 and earlier) or the newer interface from the contract (available for Symfony 4.3 and newer). The runtime check replaces the typehint that can't be enforced across multiple Symfony major versions.
There was a problem hiding this comment.
Oh yeah I didn't realize you added ! on both conditions. I thought you wanted to force the use of Contracts.
But since the translation interface param is injected with DI, I am not sure this check is very useful.
There was a problem hiding this comment.
@Seb33300 the services.xml just passes in the @translator service ... so in 4.4 it'll be the previous class... in 5.0 it'll be the new contracts-interface.
There was a problem hiding this comment.
I know and this means this cannot be something else than a TranslatorInterface.
The the check is quite useless, except if the developer override the configuration to pass something else.
If you really want to keep this check I think this is better to place it at the beginning of the method.
There was a problem hiding this comment.
I don't think the contracts interface existed in 4.1 though; https://symfony.com/doc/4.1/translation.html
Datatable/AbstractDatatable.php
Outdated
| AuthorizationCheckerInterface $authorizationChecker, | ||
| TokenStorageInterface $securityToken, | ||
| TranslatorInterface $translator, | ||
| object $translator, |
There was a problem hiding this comment.
The Composer manifest says PHP 7.1 is the minimum version, so the object typehint isn't available.
There was a problem hiding this comment.
Cheers for the feedback - now changed :)
| "symfony/dependency-injection": "^3.4|^4.1|^5.0", | ||
| "symfony/http-foundation": "^3.4|^4.1|^5.0", | ||
| "symfony/http-kernel": "^3.4|^4.1|^5.0", | ||
| "symfony/framework-bundle": "^3.4|^4.1|^5.0", |
There was a problem hiding this comment.
Do you know if there is any reason to not be compatible with Symfony 4.0?
There was a problem hiding this comment.
Change made here for security reasons: #885
Not sure if its the responsibility of this bundle to force people to upgrade because this version of symfony is not maintained anymore.
IMH we should keep compatibility with 4.0 and let people upgrade when they can.
There was a problem hiding this comment.
Dunno, but it's a bit out of scope for this PR ... i've only just expanded the existing composer requirements to 5.0 and added the extra components based on the existing 4.1 requirements for the existing components.
|
Can someone confirm if this PR worked properly for him? |
|
Can this be merged and tagged? |
6 participants
These changes allow Symfony 5 to be used as well as Twig 3 (the namespaced classes are available in Twig 2 already)