Skip to content

Remove deprecated code#6210

Merged
greg0ire merged 2 commits intosonata-project:masterfrom
jordisala1991:major/remove-deprecated-code
Aug 4, 2020
Merged

Remove deprecated code#6210
greg0ire merged 2 commits intosonata-project:masterfrom
jordisala1991:major/remove-deprecated-code

Conversation

@jordisala1991
Copy link
Member

@jordisala1991 jordisala1991 commented Jul 18, 2020

Subject

I am targeting this branch, because this is a BC Break

Changelog

### Added
- Add `RouteCollectionInterface`.
### Changed
- Class that were marked as final on Sonata 3, are now really final and cannot be extended anymore.
### Removed
- Deprecated code has been removed. You should see the UPGRADE-4.0 file to see what changed

@jordisala1991 jordisala1991 force-pushed the major/remove-deprecated-code branch 3 times, most recently from 8a021c0 to 02f45e8 Compare July 18, 2020 16:58
@jordisala1991

This comment has been minimized.

@VincentLanglet

This comment has been minimized.

@jordisala1991 jordisala1991 force-pushed the major/remove-deprecated-code branch from 02f45e8 to bb7ba5b Compare July 18, 2020 18:47
@jordisala1991
Copy link
Member Author

The remaining fails are related to the translator...

On the other hand, I applied almost all the @Final since 3.x but I cannot do all because we are mocking them and some of them do not have interfaces, should we introduce them? should I try to not mock those classes instead? @VincentLanglet

And there is another NEXT_MAJOR comment that I dont know how to solve, maybe you know:

    /**
     * NEXT_MAJOR: Decide the type declaration for the $name argument, since it is
     * passed as argument 1 for `SecurityHandlerInterface::isGranted()`, which
     * accepts string and array.
     */
    public function isGranted($name, ?object $object = null): bool

and

    /**
     * NEXT_MAJOR: Decide the type declaration for the $model argument, since it is
     * passed as argument 1 for `ModelManagerInterface::getUrlSafeIdentifier()`, which
     * accepts null.
     */
    public function getUrlSafeIdentifier($model): ?string

@VincentLanglet
Copy link
Member

@jordisala1991 You're still having an issue with phpstan:

Project config file at path /github/workspace/.phpstan/phpstan.neon.dist does not exist.

And I think you can fix

PHP Fatal error:  Declaration of Sonata\AdminBundle\Route\RouteCollection::add(string $name, ?string $pattern = NULL, array $defaults = Array, array $requirements = Array, array $options = Array, string $host = '', array $schemes = Array, array $methods = Array, string $condition = ''): Sonata\AdminBundle\Route\RouteCollection must be compatible with Sonata\AdminBundle\Route\RouteCollectionInterface::add(string $name, ?string $pattern = NULL, array $defaults = Array, array $requirements = Array, array $options = Array, string $host = '', array $schemes = Array, array $methods = Array, string $condition = ''): Sonata\AdminBundle\Route\RouteCollectionInterface in /home/travis/build/sonata-project/SonataAdminBundle/src/Route/RouteCollection.php on line 21

On the other hand, I applied almost all the @Final since 3.x but I cannot do all because we are mocking them and some of them do not have interfaces, should we introduce them? should I try to not mock those classes instead? @VincentLanglet

I already found some issue like this, see #6005 (comment)

Depending on the situation I would promote an Interface.
But new interface could be created in 3.x.

And there is another NEXT_MAJOR comment that I dont know how to solve, maybe you know:

    /**
     * NEXT_MAJOR: Decide the type declaration for the $name argument, since it is
     * passed as argument 1 for `SecurityHandlerInterface::isGranted()`, which
     * accepts string and array.
     */
    public function isGranted($name, ?object $object = null): bool

and

    /**
     * NEXT_MAJOR: Decide the type declaration for the $model argument, since it is
     * passed as argument 1 for `ModelManagerInterface::getUrlSafeIdentifier()`, which
     * accepts null.
     */
    public function getUrlSafeIdentifier($model): ?string

I think it's related to #6005 (comment)

When we tried to add typehint, the type wasn't always clear.
IMHO, this should wait another PR.
Moreover we'll have to add every missing typehint in another PR, so we'll work on these comments.

@VincentLanglet VincentLanglet requested a review from a team July 18, 2020 19:04
@jordisala1991 jordisala1991 force-pushed the major/remove-deprecated-code branch from bb7ba5b to bd06d50 Compare July 19, 2020 08:30
@jordisala1991 jordisala1991 force-pushed the major/remove-deprecated-code branch from bd06d50 to 79bd5e1 Compare July 19, 2020 08:34
@jordisala1991
Copy link
Member Author

All test fixed, there are only 1 failing because of the export thing (I think I fixed some wrong code, but test do not pass because now the labels are not translated here).

The thing is, are those labels translated somewhere else? @core23 do you know?

If I have to revert this change, it will be huge, because I will have to recover a lot of deprecated things just to make this test pass

@jordisala1991 jordisala1991 force-pushed the major/remove-deprecated-code branch 2 times, most recently from 64be20d to 6502494 Compare July 19, 2020 09:34
@VincentLanglet

This comment has been minimized.

@VincentLanglet
Copy link
Member

A changelog/upgrade note will be needed for some changes

  • New RouteCollectionInterface
  • The use of RouteCollectionInterface instead of RouteCollection for typehint
  • Removal of AbstractAdmin::$baseCodeRoute
  • Removal of AbstractAdmin::$translator
  • Removal of AbstractAdmin::$loader['view_fields'] and AbstractAdmin::$loader['view_groups']
  • Removal of AbstractAdmin::$datagridValues and how to upgrade the code
  • Removal of configureQuery context
  • Interface new methods
  • Removal of TemplateRegistry deprecated constant

And maybe others...

@jordisala1991 jordisala1991 force-pushed the major/remove-deprecated-code branch from 33b580c to 91d0a3d Compare July 19, 2020 10:00
@jordisala1991

This comment has been minimized.

@jordisala1991 jordisala1991 force-pushed the major/remove-deprecated-code branch from 91d0a3d to d1e79a8 Compare July 19, 2020 10:10
@jordisala1991
Copy link
Member Author

jordisala1991 commented Jul 19, 2020

A changelog/upgrade note will be needed for some changes

  • New RouteCollectionInterface
  • The use of RouteCollectionInterface instead of RouteCollection for typehint
  • Removal of AbstractAdmin::$baseCodeRoute
  • Removal of AbstractAdmin::$translator
  • Removal of AbstractAdmin::$loader['view_fields'] and AbstractAdmin::$loader['view_groups']
  • Removal of AbstractAdmin::$datagridValues and how to upgrade the code
  • Removal of configureQuery context
  • Interface new methods
  • Removal of TemplateRegistry deprecated constant

And maybe others...

I think on the upgrade note side we are already covering most of the things because we have this:

## Deprecations

All the deprecated code introduced on 3.x is removed on 4.0.

Please read [3.x](https://github.com/sonata-project/SonataAdminBundle/tree/3.x) upgrade guides for more information.

See also the [diff code](https://github.com/sonata-project/SonataAdminBundle/compare/3.x...4.0.0).

@jordisala1991 jordisala1991 force-pushed the major/remove-deprecated-code branch from d1e79a8 to d795cdf Compare July 19, 2020 10:16
@wbloszyk
Copy link
Member

wbloszyk commented Aug 4, 2020

I think we should do something with Datagrid in AdminBundle and DatagridBundle to keep some standard. Mybe we should consider sonata-project/dev-kit#658 one more time?

IMHO we could require DatagridBundle in AdminBundle and remove AdminBundle/Datagrid.
That's why I started sonata-project/SonataDatagridBundle#239
But I would prefer to release 4.0 before this work.

I haven't so much knowlage about it but think we should avoid using two diffrence datagrid. It is confusing.
IMHO we should try to move tools for creating CRUD to seperate bundles. In AdminBundle we should require them and use to easy create Admin Panel like now. This will allow use CRUD based on sonata in front side and AdminBundle will be easier to contribute. Anyway we should focus on quick major release. Lets keep it for now and add it to 5.0 milestone.

sonata-project/dev-kit#658 would be a better solution, but I never took the time to do it. Best moment could be just after 4.0 release, when interfaces are up to date.

IMHO it should be add to Sonata 5 milestones.

@jordisala1991
Copy link
Member Author

I like the @VincentLanglet idea of moving to DatagridBundle, but anyway it is outside the scope of this PR. (which is removing already deprecated code from 3.x 😅)

@jordisala1991 jordisala1991 requested a review from phansys August 4, 2020 13:16
VincentLanglet
VincentLanglet previously approved these changes Aug 4, 2020
wbloszyk
wbloszyk previously approved these changes Aug 4, 2020
Copy link
Member

@wbloszyk wbloszyk left a comment

Choose a reason for hiding this comment

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

It's look OK, but will be nice to replace controller:

<route id="sonata_admin_search" path="/search">
    <default key="_controller">Sonata\AdminBundle\Action\SearchAction</default>
</route>

to:

<route id="sonata_admin_search" path="/search" controller="sonata.admin.action.search"/>

@jordisala1991
Copy link
Member Author

I think we have already make to much changes on this PR, I would like to avoid doing more things if they are not needed (I plan on doing several PRs after this one, but they are currently blocked by this one.

@jordisala1991 jordisala1991 requested a review from a team August 4, 2020 14:13
@VincentLanglet
Copy link
Member

I agree.

The question is not "are some others changes needed ?" but "are all the made changes ok ?".

@jordisala1991 jordisala1991 dismissed stale reviews from wbloszyk and VincentLanglet via e424142 August 4, 2020 14:18
@jordisala1991 jordisala1991 force-pushed the major/remove-deprecated-code branch from 3d9d2e9 to e424142 Compare August 4, 2020 14:18
phansys
phansys previously approved these changes Aug 4, 2020
Copy link
Member

@phansys phansys left a comment

Choose a reason for hiding this comment

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

I just left a small suggestion at UPGRADE-3.x.md. I think this is RTM.

@jordisala1991
Copy link
Member Author

Well, they are correct to me, but that's why we are here, to review them. I will give another round myself too.

@jordisala1991
Copy link
Member Author

Do you agree on squash until this point? @VincentLanglet ? To keep this PR more clean

@VincentLanglet
Copy link
Member

Sure

@jordisala1991 jordisala1991 force-pushed the major/remove-deprecated-code branch from 5b6bb29 to d2edd54 Compare August 4, 2020 14:34
@jordisala1991
Copy link
Member Author

jordisala1991 commented Aug 4, 2020

Squashed into 2 commits, first one is the big one with everything until this point, and second one is a removal of an unused property derived from the deprecation of help here: https://github.com/sonata-project/SonataAdminBundle/pull/6215/files#diff-403e5bfbb6be15413148a2bba5343aff

@jordisala1991 jordisala1991 requested a review from a team August 4, 2020 15:17
@greg0ire greg0ire merged commit 4f95ffa into sonata-project:master Aug 4, 2020
@greg0ire
Copy link
Contributor

greg0ire commented Aug 4, 2020

Thanks @jordisala1991 !

@jordisala1991 jordisala1991 deleted the major/remove-deprecated-code branch August 5, 2020 13:25
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.

7 participants