-
Notifications
You must be signed in to change notification settings - Fork 283
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
Update Subscribers with #[AsDoctrineListener] #742
base: master
Are you sure you want to change the base?
Conversation
b2cf31d
to
64c49b5
Compare
I updated the tests some more and added support for Symfony 7. This kind of takes the PR #750 into account aswell. |
When it will be released? |
Hello 👋 Any updates about this PR? 😀 |
Nothing so far 😕I use my fork on multiple projects waiting for it to get merged |
Thanks a lot for the PR! |
I really don't understand... What is the problem with releasing this one very important feature? It is just a few minutes... |
@jgrygierek The problem is, that this library does not seem to have any active maintainers for quite some time. See #711 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution!
I'm not super comfortable with this bundle as I don't use it myself, I mainly left you compatibility and maintainability comments.
In order to avoid conflicts, it would be better to avoid to change variables names (unless it has a real benefit in doing so).
php-version: 8.0 | ||
php-version: 8.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be reverted as Symfony 6.0 still supports PHP 8.0.
If you want to add a test on PHP 8.1 you should add it instead of just change it to it IMHO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
symfony 6.0 is "Unmaintained" https://symfony.com/releases/6.0
symfony 6.4 is maintained ATM and: "Requires: PHP 8.1.0 or higher"
php-version: 8.0 | ||
php-version: 8.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be reverted as Symfony 6.0 still supports PHP 8.0.
If you want to add a test on PHP 8.1 you should add it instead of just change it to it IMHO.
php: | ||
- 8.0 | ||
operating-system: [ ubuntu-latest ] | ||
php-versions: [ '8.1', '8.3' ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should still test php 8.0 as well for the same reasons.
"php": ">=8.0", | ||
"php": ">=8.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be reverted as Symfony 6 still supports PHP 8.0 unless there is a reason for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would keep $entity
instead of changing it to $object
as it's more specific to this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would keep $entity
instead of changing it to $object
as it's more specific to this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would keep $entity
instead of changing it to $object
as it's more specific to this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why changing variable names?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid that this change is not compatible to both Symfony 6 and Symfony 7.
"symfony/cache": "^6.0|^7.0", | ||
"symfony/dependency-injection": "^6.0|^7.0", | ||
"symfony/http-kernel": "^6.0|^7.0", | ||
"symfony/security-bundle": "^6.0|^7.0", | ||
"symfony/framework-bundle": "^6.0|^7.0", | ||
"symfony/string": "^6.0|^7.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are versions 6.0-6.3, which are EOL, supported, while LTS version 5.4, which has support until November 2024 and security support until November 2025, is not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can be decided to introduce a breaking change in the library (like this one) and create a new major version while still supporting 5.4 (patches only) in the previous major version.
The fact that we bump the requirements here does not mean that the library stops supporting Symfony 5.4.
Supporting 6.0-6.3 makes sense as well unless there are some constraint that are creating us issues IMHO.
When writing libraries it's better to give a wider support of versions if there are no real reasons not to do it IMHO, not everyone might have upgraded yet to the latest 6.x version.
Thanks for the reply. I will take another look at it when I get back from vacation in two weeks. |
Any news? |
the package is dead? |
To remove the deprecation from implementing the EventSubscriberInterface this PR is to use the #[AsDoctrineListener] attribute instead.
To do so I set the
doctrine/doctrine-bundle
to^2.7.2
.Running the cs-fixer, phpstan and rector showed a couple of errors so I updated Rector and implemented some of the changes that those tools required.
There were errors in the github workflows so I updated the versions of the actions accordingly.
If there is more to update let me know I would be happy to help.
Looking at the current PRs this seems to be a combination of
#739
#741
#738
#736