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.

Sign up for GitHub

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

Add support for symfony contracts translator interface #6342

Closed
wants to merge 9 commits into from
Closed

Add support for symfony contracts translator interface #6342

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
41ade71
Add support for contracts translator interface to prepare for symfony 5
dmaicher Aug 28, 2020
007c5e1
add one more test
dmaicher Aug 30, 2020
5eb6cab
add one more test
dmaicher Aug 30, 2020
a7a88e1
ignore test when legacy interface does not exist
dmaicher Aug 30, 2020
96ae31c
support transChoice with contracts interface
dmaicher Aug 30, 2020
b9c0db5
fix CS
dmaicher Aug 30, 2020
ad2ed34
change AdminInterface
dmaicher Aug 30, 2020
b1facc4
Apply suggestions from code review
dmaicher Aug 30, 2020
296a8fd
fixes
dmaicher Aug 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .phpstan/phpstan-baseline.neon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ parameters:
-
# will be fixed in v4. Code is marked as deprecated
message: "#^Result of \\&\\& is always false\\.$#"
count: 1
count: 2
path: ../src/Admin/AbstractAdmin.php

-
Expand Down
27 changes: 23 additions & 4 deletions src/Admin/AbstractAdmin.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,10 @@
use Symfony\Component\Routing\Generator\UrlGeneratorInterface as RoutingUrlGeneratorInterface;
use Symfony\Component\Security\Acl\Model\DomainObjectInterface;
use Symfony\Component\Security\Core\Exception\AccessDeniedException;
use Symfony\Component\Translation\TranslatorInterface;
use Symfony\Component\Translation\TranslatorInterface as LegacyTranslatorInterface;
use Symfony\Component\Validator\Mapping\GenericMetadata;
use Symfony\Component\Validator\Validator\ValidatorInterface;
use Symfony\Contracts\Translation\TranslatorInterface;
Copy link
Member

Choose a reason for hiding this comment

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

Isnt this available on sf4.4?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it is

Copy link
Member

@jordisala1991 jordisala1991 Aug 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

So in master the compat code can be droped safely right? if thats the case we should add next major cments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes indeed. On master it can be typehinted with Symfony\Contracts\Translation\TranslatorInterface and we can drop all usages of the old legacy interface

Copy link
Contributor Author

@dmaicher dmaicher Aug 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

But actually on master most usages should be fully removed anyway 😊

See https://github.com/sonata-project/SonataAdminBundle/pull/6342/files#diff-61e6d1f8fafe87b868adbe28ce0ba8f6R311

https://github.com/sonata-project/SonataAdminBundle/pull/6342/files#diff-61e6d1f8fafe87b868adbe28ce0ba8f6R2467

https://github.com/sonata-project/SonataAdminBundle/pull/6342/files#diff-30aa995d1ac562ec3c1e60958ab6032fR49

etc


/**
* @author Thomas Rabaix <[email protected]>
Expand Down Expand Up @@ -309,7 +310,7 @@ abstract class AbstractAdmin implements AdminInterface, DomainObjectInterface, A
*
* NEXT_MAJOR: remove this property
*
* @var \Symfony\Component\Translation\TranslatorInterface
* @var TranslatorInterface|LegacyTranslatorInterface
*
* @deprecated since sonata-project/admin-bundle 3.9, to be removed with 4.0
*/
Expand Down Expand Up @@ -2443,7 +2444,11 @@ public function transChoice($id, $count, array $parameters = [], $domain = null,

$domain = $domain ?: $this->getTranslationDomain();

return $this->translator->transChoice($id, $count, $parameters, $domain, $locale);
if ($this->translator instanceof LegacyTranslatorInterface) {
return $this->translator->transChoice($id, $count, $parameters, $domain, $locale);
}

return $this->translator->trans($id, ['%count%' => $count] + $parameters, $domain, $locale);
dmaicher marked this conversation as resolved.
Show resolved Hide resolved
}

public function setTranslationDomain($translationDomain)
Expand All @@ -2461,9 +2466,11 @@ public function getTranslationDomain()
*
* NEXT_MAJOR: remove this method
*
* @param LegacyTranslatorInterface|TranslatorInterface $translator
*
* @deprecated since sonata-project/admin-bundle 3.9, to be removed with 4.0
*/
public function setTranslator(TranslatorInterface $translator)
public function setTranslator(object $translator)
Copy link
Member

@franmomu franmomu Sep 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Would not be this BC-break for someone extending the AbstractAdmin and also implementing this method?

Copy link
Member

Choose a reason for hiding this comment

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

https://sandbox.onlinephpfunctions.com/code/33e12ee18a6acc787333047f632e5dd6a0367aef

It is possible to change type hint to more restricted, anyway you will get warnings.
Change type to less restricted will throw Error.

Technicly this change will be BC-break but will work correct in production. I think upgrade note will be enought.

Copy link
Member

Choose a reason for hiding this comment

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

@sonata-project/contributors WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but will work correct in production

that is not necessarily correct. On some of my projects even warnings will throw exceptions on production. Depends how you configure it.

So I think indeed this is a BC break 😢 good catch @franmomu

How can we avoid it? 😕

Copy link
Contributor Author

@dmaicher dmaicher Sep 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Some idea: What if we rely on __call for it?

A quick prototype

public function __call($name, $arguments)
{
    if ($name !== 'setTranslator') {
        throw new \BadMethodCallException('...');
    }

    @trigger_error(
        'The AbstractAdmin::setTranslator method is deprecated since version 3.9 and will be removed in 4.0.',
        E_USER_DEPRECATED
    );

    if (!is_array($arguments)
        || count($arguments) === 0
        || (!($translator = $arguments[0]) instanceof LegacyTranslatorInterface && !$translator instanceof TranslatorInterface)
    ) {
        throw new \TypeError('...');
    }

    $this->translator = $translator;
}

This means we could remove the setTranslator method and handle it like this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree, there are more and more BC-breaks, we should not be afraid to move those to the next major for any reason. If we are afraid, it means we don't release majors often enough, and that's the issue we should fix, maybe by releasing one major per year at a given date.

Copy link
Member

Choose a reason for hiding this comment

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

namespace Symfony\Component\Translation;

if (!interface_exists('Symfony\Component\Translation\TranslatorInterface')) {
    /**
     * @deprecated since sonata-project/admin-bundle 3.x, to be removed in 4.0. Use \Symfony\Contracts\Translation\TranslatorInterface instead.
     */
    class_alias('Symfony\Contracts\Translation\TranslatorInterface', 'Symfony\Component\Translation\TranslatorInterface');
}

WDYT about this solution?

Based on: https://github.com/sonata-project/SonataBlockBundle/blob/3.x/src/Resources/stubs/symfony2.php

Copy link
Member

Choose a reason for hiding this comment

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

IMO Add support for Symfony5 in Sonata3 will be awesome. Anyway force user to upgrade Sonata to v4 if they want use Symfony5 is also good thing.

One important question: How much work with BC-break must be done in Sonata3 to support Symfony5?

Copy link
Member

Choose a reason for hiding this comment

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

I agree about focusing on 4.0, but TBH I have no idea how far we are from having a stable, battle-tested release of 4.0 and some of the changes to be compatible with Symfony 5 have to be done anyway, so IMO I'd support for Symfony 5 in 3.x.

In this particular case, apparently (#6213 (comment)) we have the same problem in 4.x (since the method hasn't been removed).

Copy link
Member

Choose a reason for hiding this comment

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

I agree about focusing on 4.0, but TBH I have no idea how far we are from having a stable, battle-tested release of 4.0

We will never know if we don't start working actively on it.

We started with @jordisala1991, and the sonata 4 just need

  • To remove some NEXT_MAJOR
  • To add typehint every where
  • To fix minor inconsistency in Interface.

And since sonata-project/SonataDoctrineORMAdminBundle#1073 we can start to make real test with master branch.

so IMO I'd support for Symfony 5 in 3.x.

But this introduce BC-break. Or will add a lot of complexity to our code which lead to difficulty to maintain/update.

{
$args = \func_get_args();
if (isset($args[1]) && $args[1]) {
Expand All @@ -2473,6 +2480,16 @@ public function setTranslator(TranslatorInterface $translator)
), E_USER_DEPRECATED);
}

if (!$translator instanceof LegacyTranslatorInterface && !$translator instanceof TranslatorInterface) {
throw new \TypeError(sprintf(
'Argument 1 passed to "%s()" must be an instance of "%s" or "%s", instance of "%s" given.',
__METHOD__,
LegacyTranslatorInterface::class,
TranslatorInterface::class,
\get_class($translator)
));
}

$this->translator = $translator;
}

Expand All @@ -2482,6 +2499,8 @@ public function setTranslator(TranslatorInterface $translator)
* NEXT_MAJOR: remove this method
*
* @deprecated since sonata-project/admin-bundle 3.9, to be removed with 4.0
*
* @return LegacyTranslatorInterface|TranslatorInterface
*/
public function getTranslator()
{
Expand Down
8 changes: 4 additions & 4 deletions src/Admin/AdminInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,9 @@
use Symfony\Component\Form\FormBuilderInterface;
use Symfony\Component\Form\FormInterface;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\Translation\TranslatorInterface;
use Symfony\Component\Translation\TranslatorInterface as LegacyTranslatorInterface;
use Symfony\Component\Validator\Validator\ValidatorInterface;
use Symfony\Contracts\Translation\TranslatorInterface;

/**
* @author Thomas Rabaix <[email protected]>
Expand Down Expand Up @@ -64,6 +65,7 @@
* @method void reorderFormGroup(string $group, array $keys)
* @method void defineFormBuilder(FormBuilderInterface $formBuilder)
* @method string getPagerType()
* @method void setTranslator(object $translator);
*/
interface AdminInterface extends AccessRegistryInterface, FieldDescriptionRegistryInterface, LifecycleHookProviderInterface, MenuBuilderInterface, ParentAdminInterface, UrlGeneratorInterface
{
Expand All @@ -90,10 +92,8 @@ public function setDatagridBuilder(DatagridBuilderInterface $datagridBuilder);
*/
public function getDatagridBuilder();

public function setTranslator(TranslatorInterface $translator);

/**
* @return TranslatorInterface
* @return TranslatorInterface|LegacyTranslatorInterface
*/
public function getTranslator();

Expand Down
17 changes: 14 additions & 3 deletions src/Form/Type/Filter/ChoiceType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@
use Symfony\Component\Form\Extension\Core\Type\ChoiceType as FormChoiceType;
use Symfony\Component\Form\FormBuilderInterface;
use Symfony\Component\OptionsResolver\OptionsResolver;
use Symfony\Component\Translation\TranslatorInterface;
use Symfony\Component\Translation\TranslatorInterface as LegacyTranslatorInterface;
use Symfony\Contracts\Translation\TranslatorInterface;

/**
* @final since sonata-project/admin-bundle 3.52
Expand Down Expand Up @@ -47,12 +48,22 @@ class ChoiceType extends AbstractType
*
* @deprecated since sonata-project/admin-bundle 3.5, to be removed with 4.0
*
* @var TranslatorInterface
* @var TranslatorInterface|LegacyTranslatorInterface
*/
protected $translator;

public function __construct(TranslatorInterface $translator)
public function __construct(object $translator)
{
if (!$translator instanceof LegacyTranslatorInterface && !$translator instanceof TranslatorInterface) {
throw new \TypeError(sprintf(
'Argument 1 passed to "%s()" must be an instance of "%s" or "%s", instance of "%s" given.',
__METHOD__,
LegacyTranslatorInterface::class,
TranslatorInterface::class,
\get_class($translator)
));
}

$this->translator = $translator;
}

Expand Down
17 changes: 14 additions & 3 deletions src/Form/Type/Filter/DateRangeType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@
use Symfony\Component\Form\AbstractType;
use Symfony\Component\Form\FormBuilderInterface;
use Symfony\Component\OptionsResolver\OptionsResolver;
use Symfony\Component\Translation\TranslatorInterface;
use Symfony\Component\Translation\TranslatorInterface as LegacyTranslatorInterface;
use Symfony\Contracts\Translation\TranslatorInterface;

/**
* @final since sonata-project/admin-bundle 3.52
Expand All @@ -42,12 +43,22 @@ class DateRangeType extends AbstractType
*
* @deprecated since sonata-project/admin-bundle 3.5, to be removed with 4.0
*
* @var TranslatorInterface
* @var TranslatorInterface|LegacyTranslatorInterface
*/
protected $translator;

public function __construct(TranslatorInterface $translator)
public function __construct(object $translator)
{
if (!$translator instanceof LegacyTranslatorInterface && !$translator instanceof TranslatorInterface) {
throw new \TypeError(sprintf(
'Argument 1 passed to "%s()" must be an instance of "%s" or "%s", instance of "%s" given.',
__METHOD__,
LegacyTranslatorInterface::class,
TranslatorInterface::class,
\get_class($translator)
));
}

$this->translator = $translator;
}

Expand Down
17 changes: 14 additions & 3 deletions src/Form/Type/Filter/DateTimeRangeType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@
use Symfony\Component\Form\AbstractType;
use Symfony\Component\Form\FormBuilderInterface;
use Symfony\Component\OptionsResolver\OptionsResolver;
use Symfony\Component\Translation\TranslatorInterface;
use Symfony\Component\Translation\TranslatorInterface as LegacyTranslatorInterface;
use Symfony\Contracts\Translation\TranslatorInterface;

/**
* @final since sonata-project/admin-bundle 3.52
Expand All @@ -42,12 +43,22 @@ class DateTimeRangeType extends AbstractType
*
* @deprecated since sonata-project/admin-bundle 3.5, to be removed with 4.0
*
* @var TranslatorInterface
* @var TranslatorInterface|LegacyTranslatorInterface
*/
protected $translator;

public function __construct(TranslatorInterface $translator)
public function __construct(object $translator)
{
if (!$translator instanceof LegacyTranslatorInterface && !$translator instanceof TranslatorInterface) {
throw new \TypeError(sprintf(
'Argument 1 passed to "%s()" must be an instance of "%s" or "%s", instance of "%s" given.',
__METHOD__,
LegacyTranslatorInterface::class,
TranslatorInterface::class,
\get_class($translator)
));
}

$this->translator = $translator;
}

Expand Down
17 changes: 14 additions & 3 deletions src/Form/Type/Filter/DateTimeType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@
use Symfony\Component\Form\Extension\Core\Type\DateTimeType as FormDateTimeType;
use Symfony\Component\Form\FormBuilderInterface;
use Symfony\Component\OptionsResolver\OptionsResolver;
use Symfony\Component\Translation\TranslatorInterface;
use Symfony\Component\Translation\TranslatorInterface as LegacyTranslatorInterface;
use Symfony\Contracts\Translation\TranslatorInterface;

/**
* @final since sonata-project/admin-bundle 3.52
Expand Down Expand Up @@ -67,12 +68,22 @@ class DateTimeType extends AbstractType
*
* @deprecated since sonata-project/admin-bundle 3.5, to be removed with 4.0
*
* @var TranslatorInterface
* @var TranslatorInterface|LegacyTranslatorInterface
*/
protected $translator;

public function __construct(TranslatorInterface $translator)
public function __construct(object $translator)
{
if (!$translator instanceof LegacyTranslatorInterface && !$translator instanceof TranslatorInterface) {
throw new \TypeError(sprintf(
'Argument 1 passed to "%s()" must be an instance of "%s" or "%s", instance of "%s" given.',
__METHOD__,
LegacyTranslatorInterface::class,
TranslatorInterface::class,
\get_class($translator)
));
}

$this->translator = $translator;
}

Expand Down
17 changes: 14 additions & 3 deletions src/Form/Type/Filter/DateType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@
use Symfony\Component\Form\Extension\Core\Type\DateType as FormDateType;
use Symfony\Component\Form\FormBuilderInterface;
use Symfony\Component\OptionsResolver\OptionsResolver;
use Symfony\Component\Translation\TranslatorInterface;
use Symfony\Component\Translation\TranslatorInterface as LegacyTranslatorInterface;
use Symfony\Contracts\Translation\TranslatorInterface;

/**
* @final since sonata-project/admin-bundle 3.52
Expand Down Expand Up @@ -67,12 +68,22 @@ class DateType extends AbstractType
*
* @deprecated since sonata-project/admin-bundle 3.5, to be removed with 4.0
*
* @var TranslatorInterface
* @var TranslatorInterface|LegacyTranslatorInterface
*/
protected $translator;

public function __construct(TranslatorInterface $translator)
public function __construct(object $translator)
{
if (!$translator instanceof LegacyTranslatorInterface && !$translator instanceof TranslatorInterface) {
throw new \TypeError(sprintf(
'Argument 1 passed to "%s()" must be an instance of "%s" or "%s", instance of "%s" given.',
__METHOD__,
LegacyTranslatorInterface::class,
TranslatorInterface::class,
\get_class($translator)
));
}

$this->translator = $translator;
}

Expand Down
17 changes: 14 additions & 3 deletions src/Form/Type/Filter/NumberType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@
use Symfony\Component\Form\Extension\Core\Type\NumberType as FormNumberType;
use Symfony\Component\Form\FormBuilderInterface;
use Symfony\Component\OptionsResolver\OptionsResolver;
use Symfony\Component\Translation\TranslatorInterface;
use Symfony\Component\Translation\TranslatorInterface as LegacyTranslatorInterface;
use Symfony\Contracts\Translation\TranslatorInterface;

/**
* @final since sonata-project/admin-bundle 3.52
Expand Down Expand Up @@ -57,12 +58,22 @@ class NumberType extends AbstractType
*
* @deprecated since sonata-project/admin-bundle 3.5, to be removed with 4.0
*
* @var TranslatorInterface
* @var TranslatorInterface|LegacyTranslatorInterface
*/
protected $translator;

public function __construct(TranslatorInterface $translator)
public function __construct(object $translator)
{
if (!$translator instanceof LegacyTranslatorInterface && !$translator instanceof TranslatorInterface) {
throw new \TypeError(sprintf(
'Argument 1 passed to "%s()" must be an instance of "%s" or "%s", instance of "%s" given.',
__METHOD__,
LegacyTranslatorInterface::class,
TranslatorInterface::class,
\get_class($translator)
));
}

$this->translator = $translator;
}

Expand Down
6 changes: 3 additions & 3 deletions src/Twig/Extension/SonataAdminExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
use Symfony\Component\Security\Acl\Voter\FieldVote;
use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface;
use Symfony\Component\Security\Core\Exception\AuthenticationCredentialsNotFoundException;
use Symfony\Component\Translation\TranslatorInterface as LegacyTranslationInterface;
use Symfony\Component\Translation\TranslatorInterface as LegacyTranslatorInterface;
use Symfony\Contracts\Translation\TranslatorInterface;
use Twig\Environment;
use Twig\Error\LoaderError;
Expand Down Expand Up @@ -104,11 +104,11 @@ public function __construct(
), E_USER_DEPRECATED);
}

if (!$translator instanceof TranslatorInterface && !$translator instanceof LegacyTranslationInterface) {
if (!$translator instanceof TranslatorInterface && !$translator instanceof LegacyTranslatorInterface) {
throw new \TypeError(sprintf(
'Argument 2 must be an instance of "%s" or preferably "%s", "%s given"',
TranslatorInterface::class,
LegacyTranslationInterface::class,
LegacyTranslatorInterface::class,
\get_class($translator)
));
}
Expand Down
Loading