Skip to content

Conversation

@jbcr
Copy link
Contributor

@jbcr jbcr commented Mar 11, 2025

Q A
Bug fix? yes
New feature? no
BC breaks? yes
Fixed issue #63

Upgrade for Sylius 2.0 and Symfony 6.4 and ^7.0

Drop Sylius 1.x support

Drop Symfony 5.x support

@jbcr jbcr force-pushed the upgrade_sylius_2.0 branch from 5568bc9 to 24c7821 Compare March 12, 2025 08:54
@jbcr
Copy link
Contributor Author

jbcr commented Mar 12, 2025

The result in admin interface:

image

image

Copy link
Contributor

@maxperei maxperei left a comment

Choose a reason for hiding this comment

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

thank you very much for your contribution @jbcr and well done 🫰

tests are not passing as you probably notice since you changed annotation to attribute you have to edit tests/PHPUnit/Fixtures/Foo.php to use php attribute

<?php

declare(strict_types=1);

namespace Tests\Synolia\SyliusGDPRPlugin\PHPUnit\Fixtures;

use Synolia\SyliusGDPRPlugin\Attribute\Anonymize;

class Foo
{
    #[Anonymize(['faker' => 'email'])]
    public $email = '';

    #[Anonymize(['value' => 'test-annonation-value'])]
    public $value;

    #[Anonymize(['value' => 'test-annonation-value-without-property'])]
    public $valueWithoutProperty;

    #[Anonymize(['faker' => 'email', 'prefix' => 'test-annotation-prefix-'])]
    public $prefix;

    #[Anonymize(['value' => 'value', 'prefix' => 'test-annotation-prefix-value-'])]
    public $prefixValue;

    #[Anonymize(['value' => 'attribute'])]
    public $mergeYamlAnnotationConfiguration;

    #[Anonymize(['value' => ['value1', 'value2']])]
    public array $arrayValue;

    #[Anonymize(['value' => '1542', 'prefix' => '1542'])]
    public int $integer;

    public $bar;
}

but this may not correct yet because configuration yaml is not merged anymore maybe we'll have to investigate on this..

the remaining is just about code style and some tiny stuff but this a great beginning

jbcr and others added 2 commits April 2, 2025 11:26
Co-authored-by: Maxime Pereira-Lima <7437661+maxperei@users.noreply.github.com>
@jbcr
Copy link
Contributor Author

jbcr commented Apr 2, 2025

but this may not correct yet because configuration yaml is not merged anymore maybe we'll have to investigate on this..

the remaining is just about code style and some tiny stuff but this a great beginning

What is the configuration loader priority?

The attribute configuration overrides the YAML configuration. But sometimes, the loader is in a different order and the test works.

I propose using the tag priority to set the loader order and fix this error.

@maxperei
Copy link
Contributor

maxperei commented Apr 2, 2025

but this may not correct yet because configuration yaml is not merged anymore maybe we'll have to investigate on this..
the remaining is just about code style and some tiny stuff but this a great beginning

What is the configuration loader priority?

The attribute configuration overrides the YAML configuration. But sometimes, the loader is in a different order and the test works.

I propose using the tag priority to set the loader order and fix this error.

sure @jbcr, LoaderChain put AttributeLoader at the end so unfortunately mergeYamlAnnotationConfiguration is not overriden but idk why because previous AnnotationLoader was loaded at the first position 🤷 btw it's a good idea to use tag priority to control that
and beyond that it will be amazing to replace CompilerPass/RegisterAnonymizationLoader.php into a #[AutowireIterator] php attribute, i'll be glad if i could help

@jbcr jbcr force-pushed the upgrade_sylius_2.0 branch 3 times, most recently from cf44204 to bdaa267 Compare April 2, 2025 13:16
@jbcr
Copy link
Contributor Author

jbcr commented Apr 2, 2025

sure @jbcr, LoaderChain put AttributeLoader at the end so unfortunately mergeYamlAnnotationConfiguration is not overriden but idk why because previous AnnotationLoader was loaded at the first position 🤷 btw it's a good idea to use tag priority to control that and beyond that it will be amazing to replace CompilerPass/RegisterAnonymizationLoader.php into a #[AutowireIterator] php attribute, i'll be glad if i could help

It's changed and all tests pass.

maxperei
maxperei previously approved these changes Apr 2, 2025
Copy link
Contributor

@maxperei maxperei left a comment

Choose a reason for hiding this comment

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

looks wonderful now many thx @jbcr

@maxperei maxperei merged commit a72126e into synolia:main Apr 2, 2025
7 checks passed
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.

2 participants