sonata-project / SonataUserBundle

Symfony SonataUserBundle
https://docs.sonata-project.org/projects/SonataUserBundle
MIT License
339 stars 488 forks source link

Interface inconsistency #1014

Closed peter-gribanov closed 4 years ago

peter-gribanov commented 6 years ago

Environment

Sonata packages

$ composer show --latest 'sonata-project/*'
sonata-project/admin-bundle              3.31.1 3.31.1 The missing Symfony Admin Generator
sonata-project/block-bundle              3.12.0 3.12.0 Symfony SonataBlockBundle
sonata-project/cache                     2.0.1  2.0.1  Cache library
sonata-project/core-bundle               3.9.1  3.9.1  Symfony SonataCoreBundle
sonata-project/datagrid-bundle           2.3.1  2.3.1  Symfony SonataDatagridBundle
sonata-project/doctrine-extensions       1.0.2  1.0.2  Doctrine2 behavioral extensions
sonata-project/doctrine-orm-admin-bundle 3.4.2  3.4.2  Symfony Sonata / Integrate Doctrine ORM into the SonataAdminBundle
sonata-project/easy-extends-bundle       2.5.0  2.5.0  Symfony SonataEasyExtendsBundle
sonata-project/exporter                  1.8.0  1.8.0  Lightweight Exporter library
sonata-project/user-bundle               4.1.1  4.1.1  Symfony SonataUserBundle

Symfony packages

$ composer show --latest 'symfony/*'
symfony/monolog-bundle     v3.1.2 v3.1.2 Symfony MonologBundle
symfony/polyfill-apcu      v1.7.0 v1.7.0 Symfony polyfill backporting apcu_* functions to lower PHP versions
symfony/polyfill-intl-icu  v1.7.0 v1.7.0 Symfony polyfill for intl's ICU-related data and classes
symfony/polyfill-mbstring  v1.7.0 v1.7.0 Symfony polyfill for the Mbstring extension
symfony/polyfill-php56     v1.7.0 v1.7.0 Symfony polyfill backporting some PHP 5.6+ features to lower PHP versions
symfony/polyfill-php70     v1.7.0 v1.7.0 Symfony polyfill backporting some PHP 7.0+ features to lower PHP versions
symfony/polyfill-php72     v1.7.0 v1.7.0 Symfony polyfill backporting some PHP 7.2+ features to lower PHP versions
symfony/polyfill-util      v1.7.0 v1.7.0 Symfony utilities for portability of PHP codes
symfony/security-acl       v3.0.1 v3.0.1 Symfony Security Component - ACL (Access Control List)
symfony/swiftmailer-bundle v3.2.0 v3.2.0 Symfony SwiftmailerBundle
symfony/symfony            v3.4.4 v4.0.4 The Symfony PHP framework

PHP version

$ php -v
PHP 7.2.1-1+ubuntu16.04.1+deb.sury.org+1 (cli) (built: Jan  5 2018 13:54:13) ( NTS )
Copyright (c) 1997-2017 The PHP Group
Zend Engine v3.2.0, Copyright (c) 1998-2017 Zend Technologies
    with Zend OPcache v7.2.1-1+ubuntu16.04.1+deb.sury.org+1, Copyright (c) 1999-2017, by Zend Technologies

Subject

I update SonataUserBundle to version 4.x and encountered a problem in UserInterface::setDateOfBirth().

It is stated that the argument is a \DateTime, but in fact it isn't and this is not checked. And at the same time we have a normal methods:

But i do not understand why created_at field can be nullable and why updated_at field can reseted to nullable.

Solution for me:

public function setDateOfBirth($date_of_birth)
{
    if ($date_of_birth instanceof \DateTime) {
        $this->date_of_birth = clone $date_of_birth;
    } else {
        $this->date_of_birth = null;
    }

    return $this;
}

But it is not good way. If you made a major release, why didn't you fix such obvious problems?

greg0ire commented 6 years ago

If you made a major release, why didn't you fix such obvious problems?

That's nice.

soullivaneuh commented 6 years ago

If you made a major release, why didn't you fix such obvious problems?

  1. Making new major is not related to bug fixes. It's better to fix bug before indeed, but not mandatory.
  2. The fact the problem is obvious is a subjective point of view.
  3. The project is open-source. Why didn't you proposed a fix for such an obvious problem (according to you)?

Thanks for reporting the bug, but you really don't need to add this kind of attack. If you don't like the work is done here, you are free to create your own bundle.

peter-gribanov commented 6 years ago

Maybe i was being rude. Sorry. :bow:

I'm a little upset by the fact that solving the problem breaks BC and i should not expect an early resolution of this problem.

I wish i had noticed this problem before. I did not use your model, but only implemented the interface, but now i'm forced to implement even those methods that i don't need. It seems that i really have to abandon your library in the near future :worried:

greg0ire commented 6 years ago

but now i'm forced to implement even those methods that i don't need.

sounds like an ISP violation :/ , and you're right, it would be hard to solve IMO

peter-gribanov commented 6 years ago

Сontinuation of the epic

I caught the error at this line:

Model class "Acme\DemoBundle\Entity\User" does not correspond to manager type "orm".

where

$actualModelClass is equile Acme\DemoBundle\Entity\User $expectedModelClass is equile Sonata\UserBundle\Entity\BaseUser

It's strange that we expect a concrete class, not an interface, although we have it.

If i extends from Sonata\UserBundle\Entity\BaseUser, i get error:

Duplicate definition of column 'created_at' on entity 'Acme\DemoBundle\Entity\User' in a field or discriminator column mapping.

If i disable all duplicates, i get this error:

SQLSTATE[42S22]: Column not found: 1054 Unknown column 't0.website' in 'field list'

Now i probably need to create a columns in DB that i not needed :worried: Is there a way to solve this problem without resorting to such major changes?

Hanmac commented 6 years ago

@peter-gribanov : the bug with not correspond is this pr #980 the check force to use Sonata classes, even if you don't need them (i use the FOSUser classed)

in my local code because this is not merged, i just disable the "checkManagerTypeToModelTypeMapping" check in the SonataUserExtension

peter-gribanov commented 6 years ago

I solved the problem for myself by removing the SonataUserBundle.

stale[bot] commented 4 years ago

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.