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.

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

DennisdeBest
Copy link

@DennisdeBest DennisdeBest commented Oct 20, 2023

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

@DennisdeBest
Copy link
Author

DennisdeBest commented Feb 25, 2024

I updated the tests some more and added support for Symfony 7.
I dropped support for Symfony5 and PHP 8.0

This kind of takes the PR #750 into account aswell.

@jgrygierek
Copy link

When it will be released?

@Enz000
Copy link

Enz000 commented Apr 10, 2024

Hello 👋

Any updates about this PR? 😀

@DennisdeBest
Copy link
Author

Hello 👋

Any updates about this PR? 😀

Nothing so far 😕I use my fork on multiple projects waiting for it to get merged

@jzecca
Copy link

jzecca commented Apr 15, 2024

Thanks a lot for the PR!
Would be great to have this released.

@jgrygierek
Copy link

I really don't understand... What is the problem with releasing this one very important feature? It is just a few minutes...

@virtualize
Copy link

@jgrygierek The problem is, that this library does not seem to have any active maintainers for quite some time. See #711

Copy link
Member

@alexpozzi alexpozzi left a 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
Copy link
Member

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.

Copy link

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

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

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

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.

composer.json Show resolved Hide resolved
Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why changing variable names?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is done by Rector automatically : RenameVariableToMatchMethodCallReturnTypeRector

Copy link
Member

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.

composer.json Outdated
Comment on lines 20 to 25
"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",

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?

Copy link
Member

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.

@DennisdeBest
Copy link
Author

@alexpozzi

Thanks for the reply.

I will take another look at it when I get back from vacation in two weeks.

@jgrygierek
Copy link

Any news?

@ernestoliberio
Copy link

the package is dead?

@azribilel
Copy link

Hello @acrobat, Any news ?

@DennisdeBest
Copy link
Author

DennisdeBest commented Jun 8, 2024

There are some changes with Symfony 6.1 which make it difficult to use 6.0 as the lowest version. I updated the composer.json to only allow symfony 6.1 and php 8.1.

The changes of some of the variables is the current Rector behaviour so we might aswell keep it like that.

If this gets merged the next thing that might have to be updated is the doctrine ORM from v2 to V3 but doing that here makes it a bit too much in one PR.

@MICMathieu
Copy link

@DennisdeBest some news about this PR ?

@jgrygierek
Copy link

Anyone alive here?

@jgrygierek
Copy link

Bump?

@DennisdeBest
Copy link
Author

@MICMathieu sorry no news so far

@jgrygierek
Copy link

?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.