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

EZP-31547: Autowiring fails for CustomTag service when using 3rd party translator bundle #1335

Merged
merged 4 commits into from
Apr 17, 2020

Conversation

mateuszbieniek
Copy link
Contributor

@mateuszbieniek mateuszbieniek commented Apr 7, 2020

Question Answer
Tickets https://jira.ez.no/browse/EZP-31547
Bug fix? yes

This is continuation for #1058

This PR adds $translator argument to EzSystems\EzPlatformAdminUi\UI\Config\Mapper\FieldType\RichText\CustomTag to prevent autowiring issues.

@mateuszbieniek
Copy link
Contributor Author

mateuszbieniek commented Apr 7, 2020

As mentioned by @webhdx #1007 (comment) here, the provided solution seems like overkill, but without we will encounter issues like EZP-31547.

Another alternative would be solution used by Symfony, but I'm not sold.

    /**
     * @var TranslatorInterface|TranslatorBagInterface
     */
    private $translator;
    /**
     * @param TranslatorInterface $translator The translator must implement TranslatorBagInterface
     */
    public function __construct(TranslatorInterface $translator)
    {
        if (!$translator instanceof TranslatorBagInterface) {
            throw new InvalidArgumentException(sprintf('The Translator "%s" must implement TranslatorInterface and TranslatorBagInterface.', \get_class($translator)));
        }

        $this->translator = $translator;
    }

WDYT?

Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

I prefer approach with two separate interfaces, as-is ATM, even though ATM it's the same Service. It's more SOLID.
Keep in mind that merge up might require extra effort due to switch to \Symfony\Contracts.

@tomaszszopinski tomaszszopinski self-assigned this Apr 17, 2020
Copy link
Contributor

@tomaszszopinski tomaszszopinski left a comment

Choose a reason for hiding this comment

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

QA approved on eZPlatform 2.5 EE with patch & diff.

@mateuszbieniek
Copy link
Contributor Author

Merge up for ezplatform-richtext:
ezsystems/ezplatform-richtext#142

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

Successfully merging this pull request may close these issues.

6 participants