search

sonata-project / SonataDoctrineORMAdminBundle

Integrate Doctrine ORM into the SonataAdminBundle
https://docs.sonata-project.org/projects/SonataDoctrineORMAdminBundle
MIT License
445 stars 344 forks source link

The order list has been changed #1010

Closed franmomu closed 4 years ago

franmomu commented 4 years ago

Environment

Sonata packages

$ composer show --latest 'sonata-project/*'
sonata-project/admin-bundle                  3.63.0 3.63.0 The missing Symfony Admin Generator
sonata-project/block-bundle                  3.18.4 4.2.0  Symfony SonataBlockBundle
sonata-project/cache                         2.0.1  2.0.1  Cache library
sonata-project/core-bundle                   3.18.0 3.18.0 Symfony SonataCoreBundle (abandoned)
sonata-project/datagrid-bundle               2.5.0  3.1.1  Symfony SonataDatagridBundle
sonata-project/doctrine-extensions           1.5.1  1.5.1  Doctrine2 behavioral extensions
sonata-project/doctrine-mongodb-admin-bundle 3.3.0  3.3.0  Symfony Sonata / Integrate Doctrine MongoDB ODM into the SonataAdminBundle
sonata-project/doctrine-orm-admin-bundle     3.15.0 3.15.0 Symfony Sonata / Integrate Doctrine ORM into the SonataAdminBundle
sonata-project/exporter                      2.2.0  2.2.0  Lightweight Exporter library
sonata-project/translation-bundle            2.4.2  2.4.2  SonataTranslationBundle

Symfony packages

$ composer show --latest 'symfony/*'
symfony/amazon-mailer              v4.4.5  v4.4.5  Symfony Amazon Mailer Bridge
symfony/asset                      v4.4.5  v4.4.5  Symfony Asset Component
symfony/browser-kit                v4.4.5  v4.4.5  Symfony BrowserKit Component
symfony/cache                      v4.4.5  v4.4.5  Symfony Cache component with PSR-6, PSR-16, and tags
symfony/cache-contracts            v2.0.1  v2.0.1  Generic abstractions related to caching
symfony/config                     v4.4.5  v4.4.5  Symfony Config Component
symfony/console                    v4.4.5  v4.4.5  Symfony Console Component
symfony/css-selector               v4.4.5  v4.4.5  Symfony CssSelector Component
symfony/debug                      v4.4.5  v4.4.5  Symfony Debug Component
symfony/debug-bundle               v4.4.5  v4.4.5  Symfony DebugBundle
symfony/debug-pack                 v1.0.7  v1.0.7  A debug pack for Symfony projects
symfony/dependency-injection       v4.4.5  v4.4.5  Symfony DependencyInjection Component
symfony/doctrine-bridge            v4.4.5  v4.4.5  Symfony Doctrine Bridge
symfony/dom-crawler                v4.4.5  v4.4.5  Symfony DomCrawler Component
symfony/dotenv                     v4.4.5  v4.4.5  Registers environment variables from a .env file
symfony/error-handler              v4.4.5  v4.4.5  Symfony ErrorHandler Component
symfony/event-dispatcher           v4.4.5  v4.4.5  Symfony EventDispatcher Component
symfony/event-dispatcher-contracts v1.1.7  v2.0.1  Generic abstractions related to dispatching event
symfony/expression-language        v4.4.5  v4.4.5  Symfony ExpressionLanguage Component
symfony/filesystem                 v4.4.5  v4.4.5  Symfony Filesystem Component
symfony/finder                     v4.4.5  v4.4.5  Symfony Finder Component
symfony/flex                       v1.6.2  v1.6.2  Composer plugin for Symfony
symfony/form                       v4.4.5  v4.4.5  Symfony Form Component
symfony/framework-bundle           v4.4.5  v4.4.5  Symfony FrameworkBundle
symfony/http-client                v4.4.5  v4.4.5  Symfony HttpClient component
symfony/http-client-contracts      v2.0.1  v2.0.1  Generic abstractions related to HTTP clients
symfony/http-foundation            v4.4.5  v4.4.5  Symfony HttpFoundation Component
symfony/http-kernel                v4.4.5  v4.4.5  Symfony HttpKernel Component
symfony/inflector                  v4.4.5  v4.4.5  Symfony Inflector Component
symfony/intl                       v4.4.5  v4.4.5  A PHP replacement layer for the C intl extension that includes additional data from the ICU library.
symfony/mailer                     v4.4.5  v4.4.5  Symfony Mailer Component
symfony/maker-bundle               v1.14.6 v1.14.6 Symfony Maker helps you create empty commands, controllers, form classes, tests and more so you can forget about writing boilerplate code.
symfony/mime                       v4.4.5  v4.4.5  A library to manipulate MIME messages
symfony/monolog-bridge             v4.4.5  v4.4.5  Symfony Monolog Bridge
symfony/monolog-bundle             v3.5.0  v3.5.0  Symfony MonologBundle
symfony/options-resolver           v4.4.5  v4.4.5  Symfony OptionsResolver Component
symfony/phpunit-bridge             v5.0.5  v5.0.5  Symfony PHPUnit Bridge
symfony/polyfill-apcu              v1.14.0 v1.14.0 Symfony polyfill backporting apcu_* functions to lower PHP versions
symfony/polyfill-intl-icu          v1.14.0 v1.14.0 Symfony polyfill for intl's ICU-related data and classes
symfony/polyfill-intl-idn          v1.14.0 v1.14.0 Symfony polyfill for intl's idn_to_ascii and idn_to_utf8 functions
symfony/polyfill-mbstring          v1.14.0 v1.14.0 Symfony polyfill for the Mbstring extension
symfony/polyfill-php72             v1.14.0 v1.14.0 Symfony polyfill backporting some PHP 7.2+ features to lower PHP versions
symfony/polyfill-php73             v1.14.0 v1.14.0 Symfony polyfill backporting some PHP 7.3+ features to lower PHP versions
symfony/polyfill-uuid              v1.14.0 v1.14.0 Symfony polyfill for uuid functions
symfony/process                    v4.4.5  v4.4.5  Symfony Process Component
symfony/profiler-pack              v1.0.4  v1.0.4  A pack for the Symfony web profiler
symfony/property-access            v4.4.5  v4.4.5  Symfony PropertyAccess Component
symfony/property-info              v4.4.5  v4.4.5  Symfony Property Info Component
symfony/proxy-manager-bridge       v4.4.5  v4.4.5  Symfony ProxyManager Bridge
symfony/routing                    v4.4.5  v4.4.5  Symfony Routing Component
symfony/security                   v4.4.5  v4.4.5  Symfony Security Component
symfony/security-acl               v3.0.4  v3.0.4  Symfony Security Component - ACL (Access Control List)
symfony/security-bundle            v4.4.5  v4.4.5  Symfony SecurityBundle
symfony/serializer                 v4.4.5  v4.4.5  Symfony Serializer Component
symfony/service-contracts          v2.0.1  v2.0.1  Generic abstractions related to writing services
symfony/stopwatch                  v4.4.5  v4.4.5  Symfony Stopwatch Component
symfony/swiftmailer-bundle         v3.4.0  v3.4.0  Symfony SwiftmailerBundle
symfony/templating                 v4.4.5  v4.4.5  Symfony Templating Component
symfony/test-pack                  v1.0.6  v1.0.6  A pack for functional and end-to-end testing within a Symfony app
symfony/translation                v4.4.5  v4.4.5  Symfony Translation Component
symfony/translation-contracts      v2.0.1  v2.0.1  Generic abstractions related to translation
symfony/twig-bridge                v4.4.5  v4.4.5  Symfony Twig Bridge
symfony/twig-bundle                v4.4.5  v4.4.5  Symfony TwigBundle
symfony/validator                  v4.4.5  v4.4.5  Symfony Validator Component
symfony/var-dumper                 v4.4.5  v4.4.5  Symfony mechanism for exploring and dumping PHP variables
symfony/var-exporter               v4.4.5  v4.4.5  A blend of var_export() + serialize() to turn any serializable data structure to plain PHP code
symfony/web-link                   v4.4.5  v4.4.5  Symfony WebLink Component
symfony/web-profiler-bundle        v4.4.5  v4.4.5  Symfony WebProfilerBundle
symfony/yaml                       v4.4.5  v4.4.5  Symfony Yaml Component

PHP version

$ php -v
PHP 7.4.3 (cli) (built: Feb 20 2020 12:23:10) ( NTS )
Copyright (c) The PHP Group
Zend Engine v3.4.0, Copyright (c) Zend Technologies
    with Zend OPcache v7.4.3, Copyright (c), by Zend Technologies
    with Xdebug v2.9.2, Copyright (c) 2002-2020, by Derick Rethans
    with blackfire v1.31.0~mac-x64-non_zts74, https://blackfire.io, by Blackfire

Subject

Since https://github.com/sonata-project/SonataDoctrineORMAdminBundle/pull/997 the order of a list changed

Steps to reproduce

Create an admin an override the createQuery method using orderBy:

public function createQuery($context = 'list')
    {
        $query = parent::createQuery();
        $query
            ->orderBy($query->getRootAliases()[0] . '.date', 'DESC');

        return $query;
    }

Expected results

To keep sorting as it was before

Actual results

It changed the order list, sorting by id first.

VincentLanglet commented 4 years ago

I don't consider this like a bug, but a bug which was fixed. Why would an order in the createQuery override the table order ?

When a user click on a column to order by 'id' or by 'name', ... It should order by 'id' or by 'name' and only after order by your custom order by 'date'.

If you want to order by date by default, you should use the datagridValues. https://symfony.com/doc/master/bundles/SonataAdminBundle/reference/action_list.html#customizing-the-sort-order

franmomu commented 4 years ago

I don't consider this like a bug, but a bug which was fixed. Why would an order in the createQuery override the table order ?

When a user click on a column to order by 'id' or by 'name', ... It should order by 'id' or by 'name' and only after order by your custom order by 'date'.

If you want to order by date by default, you should use the datagridValues. https://symfony.com/doc/master/bundles/SonataAdminBundle/reference/action_list.html#customizing-the-sort-order

My point was that we shouldn't change something that was working in specific way for a long time without giving some warning first, even if it's a bugfix.

The example I shown was a basic one, but the same applies to multiple field sorting or sorting by COUNT for example.

VincentLanglet commented 4 years ago

My point was that we shouldn't change something that was working in specific way for a long time without giving some warning first, even if it's a bugfix.

What do you propose ? I added a Fixed changelog https://github.com/sonata-project/SonataDoctrineORMAdminBundle/releases/tag/3.15.0.

The example I shown was a basic one, but the same applies to multiple field sorting or sorting by COUNT for example.

This was never documented as supported. It was just a hack some users found out.

There was a discussion about the expected behaviour of _sort_by, custom orderBy, ... here https://github.com/sonata-project/SonataAdminBundle/issues/5929#issuecomment-599680252

The only advice I could give is to override the modelManager this way. https://github.com/sonata-project/SonataDoctrineORMAdminBundle/pull/1004

With this PR we will have both the ability to use custom sortBy and ordering by a column in the next major version.

franmomu commented 4 years ago

My point was that we shouldn't change something that was working in specific way for a long time without giving some warning first, even if it's a bugfix.

What do you propose ?

IMO the new behaviour should be optional and enabled by the user or should be reverted.

I added a Fixed changelog https://github.com/sonata-project/SonataDoctrineORMAdminBundle/releases/tag/3.15.0.

The example I shown was a basic one, but the same applies to multiple field sorting or sorting by COUNT for example.

This was never documented as supported. It was just a hack some users found out.

I don't think this is a hack... it's where you have access to the query.

There was a discussion about the expected behaviour of _sort_by, custom orderBy, ... here sonata-project/SonataAdminBundle#5929 (comment)

The only advice I could give is to override the modelManager this way.

1004

With this PR we will have both the ability to use custom sortBy and ordering by a column in the next major version.

VincentLanglet commented 4 years ago

My point was that we shouldn't change something that was working in specific way for a long time without giving some warning first, even if it's a bugfix.

What do you propose ?

IMO the new behaviour should be optional and enabled by the user or should be reverted.

So you're not asking for a warning, you're asking for a revert. That's different. I strongly disagree with this. It's not because a bug was here for a long time that the fix should be optional, or reverted.

When using orderBy in the query, you couldn't use the table correctly since you couldn't order by the column. If you don't want to use the column order feature, just remove the default value from the ManagerInterface or the datagridValues. I've made PR to allow the null value.

There are still solutions to have the behaviour you're looking for. But there was no solution to have the correct behaviour before.

I added a Fixed changelog https://github.com/sonata-project/SonataDoctrineORMAdminBundle/releases/tag/3.15.0.

The example I shown was a basic one, but the same applies to multiple field sorting or sorting by COUNT for example.

This was never documented as supported. It was just a hack some users found out.

I don't think this is a hack... it's where you have access to the query.

But it's written nowhere that the query order would be applied before the column order.

franmomu commented 4 years ago

My point was that we shouldn't change something that was working in specific way for a long time without giving some warning first, even if it's a bugfix.

What do you propose ?

IMO the new behaviour should be optional and enabled by the user or should be reverted.

So you're not asking for a warning, you're asking for a revert. That's different. I strongly disagree with this. It's not because a bug was here for a long time that the fix should be optional, or reverted.

I'm asking for introducing the new behaviour as optional and disabled by default...

When using orderBy in the query, you couldn't use the table correctly since you couldn't order by the column. If you don't want to use the column order feature, just remove the default value from the ManagerInterface or the datagridValues. I've made PR to allow the null value.

There are still solutions to have the behaviour you're looking for. But there was no solution to have the correct behaviour before.

I added a Fixed changelog https://github.com/sonata-project/SonataDoctrineORMAdminBundle/releases/tag/3.15.0.

The example I shown was a basic one, but the same applies to multiple field sorting or sorting by COUNT for example.

This was never documented as supported. It was just a hack some users found out.

I don't think this is a hack... it's where you have access to the query.

But it's written nowhere that the query order would be applied before the column order.

But we're not going to agree here, so maybe it's better if someone else can help us. WDYT @sonata-project/contributors ?

VincentLanglet commented 4 years ago

I'm asking for introducing the new behaviour as optional and disabled by default...

It's like asking for an option with_bug with true as the default value.

Plus I already gave you explanation about how to get the behaviour you wanted. So this option is not necessary. The best which could be done would be a HOWTO somewhere.

But we're not going to agree here, so maybe it's better if someone else can help us. WDYT @sonata-project/contributors ?

In case you didn't see, I gave you another opinion. https://github.com/sonata-project/SonataAdminBundle/issues/5929#issuecomment-599680252

if the user clicks the email column, they should see a listing sorted by email, then last name, then first name, then primary key(s)

franmomu commented 4 years ago

I'm not talking here about how it should work... I'm talking about users that use a library and suddenly when they upgrade the library, it shows different results... To me change that behaviour even if it's a bugfix is breaking BC (also taking into account that has worked like this for years and the bug is not critical).

VincentLanglet commented 4 years ago

To me change that behaviour even if it's a bugfix is breaking BC.

By definition a bugfix will always change the behaviour of something.

Let's just take the last bugfix on this project https://github.com/sonata-project/SonataDoctrineORMAdminBundle/pull/1009

Before I could filter my list and looking for the id 3.3, now I can't. Should I call this a BC-break then ? This bug was also here for years and it was not critical. Should we add an option too ?

I'm not talking here about how it should work...

It's should only be the debate. The table should always be able to sort => Bugfix => Patch There si no BC bugfix and BC-break bugfix.

franmomu commented 4 years ago

Well I don't think it's the same. Anyway, we have different opinions and that's perfectly fine, let other people decide what to do here.

yhf8377 commented 4 years ago

I am here because we just traced down the cause of lots of test failures in our project was the change introduced to fix issue #997.

I sided with @franmomu. A fix introducing other bugs is not a true fix. As long as the previous method is allowed, we can not afford a hard fail. A deprecation warning will be helpful in this case.

VincentLanglet commented 4 years ago

A fix introducing other bugs is not a true fix.

What you're calling bugs are not. It's not because you're used to a wrong behaviour (and you wrote test on it), that fixing this wrong behaviour should be called a "bug". The fact the list was not updated when the user click on the column to sort the list, this was a bug.

As long as the previous method is allowed, we can not afford a hard fail.

Adding sortOrder is still allowed, it's just applied after the table sortBy as expected for the user.

If you don't want to sort by the column, you should not set a sortBy value, as already explained in this topic. It takes less than 10 lines to do it the right way and will certainly be the default behaviour in next major. It's not because the bug was in some way useful to you that should call the fix a hard fail.

A deprecation warning will be helpful in this case.

There is no way to add a deprecation warning since you can still use custom addOrderBy, but in the right way, which apply AFTER the table sort.

VincentLanglet commented 4 years ago

If we look at others ORM, we can see that there was not intended to add orderBy in the createQuery.

In DoctrinePhpcr, if you use an oderBy in the createQuery, it's overriden by the orderBy here https://github.com/sonata-project/SonataDoctrinePhpcrAdminBundle/blob/2.x/src/Datagrid/ProxyQuery.php#L137.

So:

This can't be called a BC-break.

greg0ire commented 4 years ago

Obligatory XKCD xkcd workflow

Riches commented 4 years ago

I appreciate both sides here, although I know from experience working with a few apps that use this bundle many admins override the default ordering, especially in filters. For now we've locked our apps at 3.14.0. Perhaps in the future a feature could be considered where table filtering is disabled altogether, enabling the orderBy overrides that people have been using?

VincentLanglet commented 4 years ago

Perhaps in the future a feature could be considered where table filtering is disabled altogether, enabling the orderBy overrides that people have been using?

It will be in 4.0. But you can still make this change on your project https://github.com/sonata-project/SonataDoctrineORMAdminBundle/pull/1004