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 to Symfony 6.3 and PHP 8.1 #44

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

Conversation

czachor
Copy link

@czachor czachor commented Mar 5, 2024

Hello,
the bundle wasn't working for me with Symfony 6 and PHP 8.2, so I made the fork. I think it could be connected to #31

What is done:

  1. PHP min. version set to 8.1.
  2. Symfony min. version set to 6.3. Probably will work with 7.0, but not tested.
  3. Controller is extending from Symfony\Bundle\FrameworkBundle\Controller\AbstractController instead of Symfony\Bundle\FrameworkBundle\Controller\AbstractController\Controller (not available in Symfony 6).
  4. Removed some deprecations.
  5. Code formatting (PSR).
  6. Commands: added returns (Command::SUCCESS).
  7. A few code micro-optimizations.
  8. Changed type comparison operators (!= -> !==).
  9. Added function's return types.
  10. Added property types.
  11. SubscriberEventand WebhookEvent: removed Symfony\Component\EventDispatcher\Event class (removed in Symfony 4?)
  12. routing.yaml: changed type: annotation to type: attributes.
  13. A lot of small changes related to language level / Symfony version upgrade.
  14. WelpMailchimpBundle extends Symfony\Component\HttpKernel\Bundle\AbstractBundle instead of Symfony\Component\HttpKernel\Bundle\Bundle.
  15. Added autowiring instead of manually getting services.

TODO:

  1. I didn't rebuild the tests: the original ones didn't work too well for me and it's a big TODO.
  2. Memory usage: syncing ~4000 users consumes a lot of memory. I'll do some tests, but I think using yield could be helpful.
  3. It's been a long time since I wrote a bundle, so there are probably a few things to improve. I'm open to suggestions and of course feel free to modify the code.

If you decide to merge, I thing it should be major release (a'la mentioned above 2.0), there are BC changes here.

Anyway it works perfect with my app syncing ~4000 users with Mailchimp.

czachor and others added 11 commits February 16, 2024 19:16
Didn't do anything in /spec and /tests.

PHP min. version set to 8.1.
Symfony min. version set to 6.3. Todo: check if lower version (6.1?) is working.
Controller is extending from AbstractController instead of Controller.
Removed some deprecations.
Added some code formatting (PSR).
Commands: added returns (Command::SUCCESS).
Micro-optimization: changed array_push($foo, $bar) to $foo[] = $bar (faster here).
Changed type comparison operators (!= -> !==).
Added function's return types.
Added property types.
SubscriberEvent and WebhookEvent: removed Symfony\Component\EventDispatcher\Event class (removed in Symfony 4?)
routing.yaml: changed "type: annotation" to "type: attributes".
A lot of small changes related to language level / Symfony version upgrade.
AbstractBundle instead of Bundle.
…\Component\Routing\Attribute\Route;

Renamed files to match Symfony conventions.
Remove not needed addAnnotatedClassesToCompile() call.
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.

1 participant