Add support for Symfony 7 #28

Zombaya · 2023-12-18T21:25:43Z

On the hackaton of symfonycon brussels 2023, I spend the day trying to add support for symfony 7, which I believe has succeeded!

I do think I need to clean things up a bit and squash a lot of commits to get a more clean history, but I wanted to create this draft PR to let you know about the work I did.

Breaking changes

I was hitting some issues with getting the unittests to run on symfony 4.4. As official support for symfony 4.4 has ended, I found this to be a good time to also drop support for symfony 4.4 and only support symfony 5.4+.

Perhaps we can create a new branch 5.x in case we need to push changes or add deprecation triggers to the current version of the project, release a 6.0.0-version and develop this in main.

Implementation

I rewrote the service-definition a bit to split up the services based on which symfony version is being used, since starting from symfony 7, the annotation-driver is no longer present.

Starting from symfony 7, you also need to register doctrine-event-listeners instead of a doctrine-event-subscriber. As I wanted to just add support for symfony 7, I just registered the subscriber as a listener, without changing the implementation.

Future implementation

I would like to rewrite the service-definition so that we would pull the detection of which fields of which classes to encrypt outside of the listener/subscriber so that we can define proper listeners which only listen for events on classes with the encrypted-attribute.

As I just wanted it to work on symfony 7, I did not implement this yet.

Work to be done

Squash commits
Cleanup?

Zombaya

I added some feedback on specific things to clarify.

Zombaya · 2023-12-18T21:27:40Z

src/DependencyInjection/DoctrineEncryptExtension.php

-            $container->setAlias('ambta_doctrine_annotation_reader','annotations.reader');
+        // Symfony 1-3
+        if (Kernel::MAJOR_VERSION < 5) {
+            throw new \RuntimeException('doctrineencryptbundle/doctrine-encrypt-bundle expects symfony-version >= 4.1!');


This message should be updated.

Is this necessary? The composer.json will prevent installing with old versions of Symfony.

Not really, more of a sanity check.

Zombaya · 2023-12-18T21:28:24Z

src/DependencyInjection/DoctrineEncryptExtension.php

+        } elseif (Kernel::MAJOR_VERSION < 7) {
+            // PHP 7.x (no attributes)
+            if (PHP_VERSION_ID < 80000) {
+                $loader->load('services_subscriber_sf4_php7.yml');


Perhaps we should rename these files to

services_subscriber_with_annotations.yml

services_subscriber_with_annotations_and_attributes.yml

service_listeners_with_attributes.yml

Zombaya · 2023-12-18T21:31:44Z

.github/workflows/ci.yml

+# Disable lowest for now
+#        include:
+#          # testing lowest PHP version with lowest dependencies
+#          - php-version: '7.2.5'
+#            dependency-versions: 'lowest'
+#            composer-options: '--prefer-lowest'


I could not get this to work, and perhaps we should also not support this any longer?

I don't mind supporting php 7.2 as long as it uses the latest versions it can (since sf 5.4 support it) but supporting it with the lowest versions possible of the supporting libraries is rather annoying.

Have you tried adding it back again after dropping support for symfony 4? Otherwise you might remove dead code

I re-added it in 0a1f3c9.

I had to bump the minimum-requirement of doctrine/doctrine-bundle from 2.0.0 to 2.0.8 for the root-project and for the sf5.4-demo-project I updated the minimum-requirement of symfony/flex to 1.21.

To have phpunit run succesfully, I had to signal phpunit-bridge to use phpunit 7 for that specific job.

Good of you to catch it, since this way we can guarantee our minimum requirements actually work. 👍

ndench · 2024-01-02T04:19:29Z

Nice work! What else needs to be done to get this ready for merging?

I'm not sure that a new major version is necessary, when the composer.json specifies that you need Symfony 5+ then a simple composer update by someone running Symfony 4 will already prevent the update. Not really my call though.

PS. This will close #8

Zombaya · 2024-02-14T15:09:20Z

I think the only thing needed is a little cleanup.

~~Personally, I think bumping the required symfony-version requires new major version, simply so we could potentially release a fix for symfony 4.x-supported versions.~~

Rethinking this, the only possible fixes I would implement for symfony 4.x-versions would be security-fixes and that would only ever be a patch version.

So my current stance would be that a minor-version would be fine.

I'll try to take some time next week to do the cleanup.

core23 · 2024-02-26T16:23:55Z

Any updates here @Zombaya or do you need support?

This is the only lib that blocks my symfony 7 update 🙃

Squashed commits ---------------- * 28c5acf 2023-12-09 Zombaya Allow symfony 7.x in root-composer-json and add demo project for symfony 7 * 47526a7 2023-12-09 Zombaya Add demo-project using symfony 7.x * e93cce7 2023-12-09 Zombaya Use path of phpunit-bridge for symfony 4.x * 2649be2 2023-12-09 Zombaya Rework service-definitions and split it up in multiple files so we support sf 4.x-7.x * d42637b 2023-12-09 Zombaya Removed symfony 4.4 demo * a05da56 2023-12-09 Zombaya Add demo-project for sf 5.4 * d185546 2023-12-09 Zombaya Replace sf 4.4 by 5.4 in CI * a05b686 2023-12-09 Zombaya Drop support for sf 4.* in composer.json * 14362ff 2023-12-09 Zombaya Allow halite 4.6 in sf 5.4-demo * 388c358 2023-12-09 Zombaya Use seperate config for php 8 and 7 in sf 5.4 demo * 507e9b4 2023-12-09 Zombaya Drop support for sf 4 in DoctrineEncryptExtension as well * 9f23159 2023-12-09 Zombaya Move dev-requirements and allow more versions of phpunit-bridge * a4e79da 2023-12-09 Zombaya Never use phpunit directly - only use the bridge * 35add17 2023-12-09 Zombaya Ignore tests in phpstan for now * 0c8d2e0 2023-12-09 Zombaya Use simple-php-unit for root-project * aff5a52 2023-12-09 Zombaya Stop forcing phpunit version for phpunit-bridge in 5.4-demo * 3172c8a 2023-12-09 Zombaya Remove forced version for php 7.2 for composer * b74e4fb 2023-12-09 Zombaya Require recent version of phpunit-bridge for root-project * 53260f6 2023-12-09 Zombaya Remove lowest for now * d414bb8 2023-12-09 Zombaya Add CI-run for demo 7.x * f58810f 2023-12-09 Zombaya Restrict CI for sf7 to php 8.2 * a1189b2 2024-02-26 Zombaya Small cleanup after review * 7ce2724 2024-02-26 Zombaya Small fix for missing rename of file

Zombaya · 2024-02-26T18:23:08Z

@r3hp1c , as far as I'm concerned, this is ready to be merged in and released. I squashed everything into a single commit.

This will drop support for symfony 4.x and up the minimal requirement to symfony 5.4 but I think that is fair since older versions are also no longer supported by symfony itself.

@core23 , thanks for wanting to help, but the only thing missing was a motivation to work after hours 🙂 Hopefully your upgrade to symfony 7 will run smoothly after this is released.

core23 · 2024-02-27T08:32:42Z

composer.json

 		"defuse/php-encryption": "^2.1",
 		"doctrine/cache": "^1.11",
 		"phpstan/phpstan": "^1.4",
 		"jetbrains/phpstorm-attributes": "^1.0",
 		"phpcompatibility/php-compatibility": "^9.3",
-		"symfony/phpunit-bridge": "^6.0"
+		"symfony/phpunit-bridge": "^6.4|^7.0"


I think you can drop symfony/phpunit-bridge 6 completly. It is mostly backward compatible

I checked the docs and you're right, we can drop the support for 6.4, especially since they are in the dev-requirements, which do not affect users of our library.

From the docs:

The PHPUnit bridge is designed to work with all maintained versions of Symfony components,
even across different major versions of them.
You should always use its very latest stable major version to get the most accurate deprecation report.

r3hp1c · 2024-02-27T16:50:56Z

@Zombaya I am happy going forward to keep support the same as Symfony itself.
Would you like to have a look at dropping "symfony/phpunit-bridge" like suggested by@core23 before I merge?

From the documentation: > The PHPUnit bridge is designed to work with all maintained versions of Symfony components, > even across different major versions of them. > You should always use its very latest stable major version to get the most accurate deprecation report.

Changes ======= * Set variable to use correct phpunit to run on lowest See symfony/symfony#52844 * Up the minimum-requirement for symfony/flex for demo/sf5.4 to 1.21 * Update minimal version of doctrine/doctrine-bundle to allow installing with prefer-lowest on sf 5.4

Zombaya

I think we're good to go now.

I added some code which should resolve the latest remarks by @core23.

Zombaya · 2024-02-29T19:38:14Z

.github/workflows/ci.yml

+# Disable lowest for now
+#        include:
+#          # testing lowest PHP version with lowest dependencies
+#          - php-version: '7.2.5'
+#            dependency-versions: 'lowest'
+#            composer-options: '--prefer-lowest'


I re-added it in 0a1f3c9.

I had to bump the minimum-requirement of doctrine/doctrine-bundle from 2.0.0 to 2.0.8 for the root-project and for the sf5.4-demo-project I updated the minimum-requirement of symfony/flex to 1.21.

To have phpunit run succesfully, I had to signal phpunit-bridge to use phpunit 7 for that specific job.

Good of you to catch it, since this way we can guarantee our minimum requirements actually work. 👍

Zombaya · 2024-02-29T19:40:16Z

composer.json

 		"defuse/php-encryption": "^2.1",
 		"doctrine/cache": "^1.11",
 		"phpstan/phpstan": "^1.4",
 		"jetbrains/phpstorm-attributes": "^1.0",
 		"phpcompatibility/php-compatibility": "^9.3",
-		"symfony/phpunit-bridge": "^6.0"
+		"symfony/phpunit-bridge": "^6.4|^7.0"


I checked the docs and you're right, we can drop the support for 6.4, especially since they are in the dev-requirements, which do not affect users of our library.

From the docs:

The PHPUnit bridge is designed to work with all maintained versions of Symfony components,
even across different major versions of them.
You should always use its very latest stable major version to get the most accurate deprecation report.

Petervw · 2024-03-01T20:10:56Z

First of all, thanks for all contributions made already! This PR seems to be ready for a merge and release. Are there any open issues we could help with to get this PR merged?

core23 · 2024-03-04T19:59:04Z

src/DependencyInjection/DoctrineEncryptExtension.php

+        }
+
+        // Symfony 5-6
+        if (Kernel::MAJOR_VERSION < 7) {


Just tested it with one of my projects where I'm using symfony 6.4, but doctrine-extension 7.

Sadly this check does not work as expected and might need some adjustments for that case

As said in #30 (comment), I think this issue should be resolved by sticking to either 6.4 or 7.0, but not both at the same time.

Zombaya commented Dec 18, 2023

View reviewed changes

r3hp1c mentioned this pull request Dec 28, 2023

Added support to encrypt and decrypt DateTime, JSON and array data types #24

Merged

Zombaya force-pushed the symfony7 branch from 7ce2724 to 8329fb6 Compare February 26, 2024 18:19

Zombaya marked this pull request as ready for review February 26, 2024 19:35

core23 reviewed Feb 27, 2024

View reviewed changes

Zombaya added 2 commits February 29, 2024 19:19

Zombaya commented Feb 29, 2024

View reviewed changes

r3hp1c merged commit c3df835 into DoctrineEncryptBundle:master Mar 2, 2024
8 checks passed

core23 reviewed Mar 4, 2024

View reviewed changes

core23 mentioned this pull request Mar 5, 2024

Fix registering doctrine subscriber for kernel 6 with doctrine-bridge 7 #30

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Symfony 7 #28

Add support for Symfony 7 #28

Zombaya commented Dec 18, 2023 •

edited

Zombaya left a comment

Zombaya Dec 18, 2023

ndench Jan 2, 2024

Zombaya Feb 26, 2024

Zombaya Dec 18, 2023

Zombaya Dec 18, 2023

core23 Feb 27, 2024

Zombaya Feb 29, 2024

ndench commented Jan 2, 2024 •

edited

Zombaya commented Feb 14, 2024 •

edited

core23 commented Feb 26, 2024

Zombaya commented Feb 26, 2024 •

edited

core23 Feb 27, 2024

Zombaya Feb 29, 2024

r3hp1c commented Feb 27, 2024 •

edited

Zombaya left a comment

Zombaya Feb 29, 2024

Zombaya Feb 29, 2024

Petervw commented Mar 1, 2024

core23 Mar 4, 2024

Zombaya Mar 7, 2024

Add support for Symfony 7 #28

Add support for Symfony 7 #28

Conversation

Zombaya commented Dec 18, 2023 • edited

Breaking changes

Implementation

Future implementation

Work to be done

Zombaya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndench commented Jan 2, 2024 • edited

Zombaya commented Feb 14, 2024 • edited

core23 commented Feb 26, 2024

Zombaya commented Feb 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r3hp1c commented Feb 27, 2024 • edited

Zombaya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Petervw commented Mar 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zombaya commented Dec 18, 2023 •

edited

ndench commented Jan 2, 2024 •

edited

Zombaya commented Feb 14, 2024 •

edited

Zombaya commented Feb 26, 2024 •

edited

r3hp1c commented Feb 27, 2024 •

edited