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

[Twig] Split SonataAdminExtension #6676

Merged
merged 9 commits into from
Jan 11, 2021

Conversation

tambait
Copy link
Contributor

@tambait tambait commented Dec 8, 2020

Subject

I am targeting this branch, because BC.

Closes #6639.

Changelog

### Added
- Added `Sonata\AdminBundle\Twig\Extension\SecurityExtension`
- Added `Sonata\AdminBundle\Twig\Extension\CanonicalizeExtension`
- Added `Sonata\AdminBundle\Twig\Extension\XEditableExtension`
- Added `Sonata\AdminBundle\Twig\Extension\RenderElementExtension`
### Deprecated
- Deprecated `SonataAdminExtension::MOMENT_UNSUPPORTED_LOCALES` constant
- Deprecated `SonataAdminExtension::setXEditableTypeMapping()` method
- Deprecated `SonataAdminExtension::getXEditableType()` method
- Deprecated `SonataAdminExtension::getXEditableChoices()` method
- Deprecated `SonataAdminExtension::getCanonicalizedLocaleForMoment()` method
- Deprecated `SonataAdminExtension::getCanonicalizedLocaleForSelect2()` method
- Deprecated `SonataAdminExtension::isGrantedAffirmative()` method
- Deprecated `SonataAdminExtension::renderListElement()` method
- Deprecated `SonataAdminExtension::renderViewElement()` method
- Deprecated `SonataAdminExtension::renderViewElementCompare()` method
- Deprecated `SonataAdminExtension::renderRelationElement()` method
- Deprecated `SonataAdminExtension::getTemplate()` method
- Deprecated `SonataAdminExtension::getTemplateRegistry()` method

@SonataCI
Copy link
Collaborator

SonataCI commented Dec 8, 2020

Could you please rebase your PR and fix merge conflicts?

@VincentLanglet
Copy link
Member

@tambait Hi !

Thanks for taking this issue.

Our policy is to do as much as possible on 3.x branch, and in this case you can.

You should

  • Add CanonicalizeExtension
  • Add SecurityExtension
  • Add XEditableExtension
  • Deprecate methods on SonataAdminExtension

Then in master we remove the deprecated methods. :)

@tambait tambait force-pushed the split-twig-extensions-6639 branch 3 times, most recently from 7a0e283 to 2c83e6c Compare December 8, 2020 19:51
@SonataCI
Copy link
Collaborator

SonataCI commented Dec 8, 2020

Could you please rebase your PR and fix merge conflicts?

@tambait tambait changed the base branch from master to 3.x December 8, 2020 19:53
@tambait
Copy link
Contributor Author

tambait commented Dec 8, 2020

@tambait Hi !

Thanks for taking this issue.

Our policy is to do as much as possible on 3.x branch, and in this case you can.

You should

* Add CanonicalizeExtension

* Add SecurityExtension

* Add XEditableExtension

* Deprecate methods on SonataAdminExtension

Then in master we remove the deprecated methods. :)

Thanks for reminding. I had this morning idea to work/merge on 3.x but diverged from it somehow :)

What about config file, Resources/config/twig.php when it should be modified?

@VincentLanglet
Copy link
Member

What about config file, Resources/config/twig.php when it should be modified?

You can declare new extension on 3.x, and add NEXT_MAJOR comment to remove the extra param from the constructor.

@tambait tambait force-pushed the split-twig-extensions-6639 branch from 2c83e6c to 1481876 Compare December 8, 2020 20:14
src/Twig/Extension/CanonicalizeExtension.php Outdated Show resolved Hide resolved
src/Twig/Extension/CanonicalizeExtension.php Outdated Show resolved Hide resolved
src/Twig/Extension/SecurityExtension.php Outdated Show resolved Hide resolved
src/Twig/Extension/XEditableExtension.php Outdated Show resolved Hide resolved
src/Twig/Extension/XEditableExtension.php Outdated Show resolved Hide resolved
@tambait tambait force-pushed the split-twig-extensions-6639 branch 3 times, most recently from 64d50db to 5ab3340 Compare December 8, 2020 20:51
@tambait
Copy link
Contributor Author

tambait commented Dec 10, 2020

@VincentLanglet can you please help with this :)

I have a series of similar errors in tests most pointing to the same lines in SonataAdminExtension. For example,

102) Sonata\AdminBundle\Tests\Twig\Extension\SonataAdminExtensionTest::testRenderListElement with data set
#100 ('<td class="sonata-ba-list-fie...n</td>', 'text', 'A very long string', array(array(10, 'More', 'Less')))
Twig\Error\RuntimeError: An exception has been thrown during the rendering of a template ("Undefined offset: 1").

/sonata/twig-extensions/src/Resources/views/CRUD/base_list_field.html.twig:43
/sonata/twig-extensions/vendor/twig/twig/src/Template.php:367
/sonata/twig-extensions/vendor/twig/twig/src/Template.php:379
/sonata/twig-extensions/vendor/twig/twig/src/TemplateWrapper.php:40
/sonata/twig-extensions/src/Twig/Extension/SonataAdminExtension.php:815
/sonata/twig-extensions/src/Twig/Extension/SonataAdminExtension.php:284
/sonata/twig-extensions/tests/Twig/Extension/SonataAdminExtensionTest.php:428

I initially wasn't sure why this happens because I didn't see that tested metods are calling any of the methods I'm deprecating but then I realized that errors are caused because I've added sonata_deprecation_mute to two filter methods, for example:

getFilters()

   [...]
    //NEXT_MAJOR remove this filter
    new TwigFilter(
        'sonata_xeditable_choices',
        [$this, 'getXEditableChoices']
    ),
    [ ...]

getXEditableChoices method

public function getXEditableChoices(FieldDescriptionInterface $fieldDescription)
{

    if ('sonata_deprecation_mute' !== \func_get_args()[1] ?? null) {
        @trigger_error(sprintf(
            'The %s method is deprecated in favor of XEditableExtension::getXEditableChoices since version 3.x
             and will be removed in 4.0.',
            __METHOD__
        ), E_USER_DEPRECATED);
    }
 [...]
}

Should I remove sonata_deprecation_mute and leave only trigger_error or I shoudl do something else?

@VincentLanglet
Copy link
Member

VincentLanglet commented Dec 10, 2020

@tambait Didn't read everything but

if ('sonata_deprecation_mute' !== (\func_get_args()[1] ?? null)) {

instead of

if ('sonata_deprecation_mute' !== \func_get_args()[1] ?? null) {

I'll make a review later

@tambait tambait force-pushed the split-twig-extensions-6639 branch from b6f3b00 to 5131f13 Compare December 10, 2020 12:48
@tambait tambait force-pushed the split-twig-extensions-6639 branch 4 times, most recently from f99fd95 to c05d7f2 Compare December 10, 2020 14:33
@franmomu
Copy link
Member

About the constructor, I don't know if we can just make it @internal (since now), do not change it and make the necessary changes in 4.x branch.

One way to avoid BC break I guess would be if the getFunctions(), getFilters(), etc methods from the new extensions return empty arrays and make that SonataAdminExtension delegate to those extensions, something like:

public function isGrantedAffirmative($role, $object = null, $field = null)
{
    if (null === $this->securityExtension) {
        $this->securityExtension = new SecurityExtension($this->securityChecker);
    }
    
    return $this->securityExtension->isGrantedAffirmative($role, $object, $field);
}

@tambait tambait force-pushed the split-twig-extensions-6639 branch from e708351 to 9d50602 Compare December 14, 2020 18:03
@tambait tambait force-pushed the split-twig-extensions-6639 branch from 7c07b19 to 0ede293 Compare January 6, 2021 10:56
@franmomu franmomu force-pushed the split-twig-extensions-6639 branch from 0ede293 to 5dd14b7 Compare January 6, 2021 19:32
@franmomu franmomu requested a review from a team January 6, 2021 19:34
@franmomu franmomu force-pushed the split-twig-extensions-6639 branch 2 times, most recently from 52dfc05 to 21fbab7 Compare January 7, 2021 08:13
@VincentLanglet
Copy link
Member

I didn't follow a lot this PR.
Is it ready for review @franmomu @tambait ?

@tambait
Copy link
Contributor Author

tambait commented Jan 9, 2021

@VincentLanglet

I didn't follow a lot this PR.
Is it ready for review @franmomu @tambait ?

franmomu was doing most of the work in recent period and few days ago he requested a review, so I believe it's ready.

franmomu requested a review from sonata-project/contributors 3 days ago

@franmomu franmomu force-pushed the split-twig-extensions-6639 branch from 21fbab7 to 0d544ba Compare January 9, 2021 21:55
@franmomu
Copy link
Member

franmomu commented Jan 9, 2021

I fixed some tests some days ago, now it's rebased and added an upgrade note. I think it's ready for review.

@VincentLanglet VincentLanglet requested review from core23 and a team January 9, 2021 22:14
@OskarStark OskarStark requested a review from franmomu January 10, 2021 08:14
@VincentLanglet
Copy link
Member

Is it ok for you @core23 ?

@VincentLanglet VincentLanglet merged commit 128941f into sonata-project:3.x Jan 11, 2021
@VincentLanglet
Copy link
Member

Thanks @tambait

@tambait tambait deleted the split-twig-extensions-6639 branch January 11, 2021 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Split SonataAdminExtension twig extension in smaller extensions
6 participants