msgphp / symfony-demo-app

A Symfony demo application with basic user management
https://github.com/msgphp/msgphp
MIT License
0 stars 0 forks source link

Creating user - returned id is incremented by one #42

Closed crabnky closed 6 years ago

crabnky commented 6 years ago

Hi, it's me again :)

I have an issue when creating new user. After I submit the form, user is created, but the id of the $user object is incremented by one - for example I created user and I can see in the database that new redord has id = 30, but the $user->id is set to 31, and I'm getting error on line with redirect (redirecting to non-existing user). Here is the controller function:

    /**
     * @Route(
     *     "/users/add",
     *     name = "users_add"
     * )
     * @param Request $request
     * @param PasswordHashingInterface $passwordHashing
     *
     * @return Response
     */
    public function index(Request $request, PasswordHashingInterface $passwordHashing)
    {
        $userForm = $this->createForm(UserFormType::class);
        $userForm->handleRequest($request);

        if ($userForm->isSubmitted() && $userForm->isValid()) {

            /** @var User $user */
            $user = $userForm->getData();

            $user->changePassword($passwordHashing->hash($userForm->get('password')->getData()));
            $user->changeEmail($userForm->get('email')->getData());

            $isEnabled = $userForm->get('isEnabled')->getData();
            if ($isEnabled) {
                $user->enable();
            } else {
                $user->disable();
            }

            $entityManager = $this->getDoctrine()->getManager();
            $entityManager->persist($user);
            $entityManager->flush();

            return $this->redirectToRoute('pms_users_view', [ 'id' => $user->getId() ]);
        }
        return $this->renderTemplate('user/add.html.twig', [
            'userForm' => $userForm->createView()
        ]);
    }

Here is the configureOptions function of my UserFormType:

    public function configureOptions(OptionsResolver $resolver)
    {
        $resolver->setDefaults([
            'data_class' => User::class,
            'empty_data' => function () {
                $password = $this->passwordHashing->hash('default_password');
                return new User(new UserId(), 'default-user@test.com', $password);
            },
            'translation_domain' => 'users',
            'attr' => [
                'novalidate' => 'novalidate'
            ]
        ]);
    }

Here is a part od my User entity related to id:

namespace App\Entity;

use Doctrine\Common\Collections\ArrayCollection;
use Doctrine\Common\Collections\Collection;
use MsgPhp\User\Entity\User as BaseUser;
use MsgPhp\User\UserId;
use MsgPhp\User\UserIdInterface;

/**
 * @Vich\Uploadable
 */
class User extends BaseUser
{
    /**
     * @var UserIdInterface
     */
    private $id;

    /**
     * Constructor
     *
     * @param UserIdInterface $id
     * @param string $email
     * @param string $password
     */
    public function __construct(UserIdInterface $id, string $email, string $password)
    {
        $this->id = $id;
        $this->credential = new EmailPassword($email, $password);
        $this->roles = new ArrayCollection();
        $this->settings = new ArrayCollection();
    }

    /**
     * @return UserIdInterface
     */
    public function getId(): UserIdInterface
    {
        return UserId::fromValue($this->id);
    }
}

Did I do something wrong? It was working fine, for some time I was working on other part of the application, and now noticed this... I updated composer in the meantime, so I have the latest Symfony (4.0.8) and msgphp is set to dev-master.

Thanks.

ro0NL commented 6 years ago

Can you try

-$this->id = $id;
+$this->id = $id->isEmpty() ? null : (int) $id->toString();

In your case the ID property value is always the primitive type (int or null), you hydrate it as UserIdInterface type from getId().

crabnky commented 6 years ago

Did not help. :( I'm not sure if this is related to your bundle - it was working fine some time ago, and last time I did not make any changes to the users. Strange.

ro0NL commented 6 years ago

Can you share msgphp config + doctrine orm mapping :)

ro0NL commented 6 years ago

Also perhaps put a dump($user) before and after $entityManager->flush(); to see if anything unexpected happens with the ID value.

crabnky commented 6 years ago

Here is the dump of user before: $entityManager->flush();:

User {#3161 ▼
  -id: null
  -firstName: "Test"
  -lastName: "Test"
  -abbreviation: null
  -birthDate: null
  -phoneExtension: null
  -language: Language {#2006 ▼
    +__isInitialized__: true
    -code: "de"
    -name: "Deutsch"
    -sortOrder: 1
     …2
  }
  -roles: ArrayCollection {#3210 ▼
    -elements: []
  }
  -settings: ArrayCollection {#3165 ▼
    -elements: []
  }
  -imageFile: null
  -imageName: null
  -imageSize: null
  -enabled: false
  -credential: EmailPassword {#8095 ▼
    -email: "test@test.com"
    -password: "$2y$13$PBozBPUemeKghIg8c3Q1sO6oj8MGi93zp5PqMf1x4ngPyuPAWb.F."
  }
  -passwordResetToken: null
  -passwordRequestedAt: null
  -confirmationToken: null
  -confirmedAt: null
  -lastUpdatedAt: null
}

And after:

User {#3161 ▼
  -id: 33
  -firstName: "Test"
  -lastName: "Test"
  -abbreviation: null
  -birthDate: null
  -phoneExtension: null
  -language: Language {#2006 ▼
    +__isInitialized__: true
    -code: "de"
    -name: "Deutsch"
    -sortOrder: 1
     …2
  }
  -roles: PersistentCollection {#8188 ▼
    -snapshot: []
    -owner: User {#3161}
    -association: array:15 [ …15]
    -em: EntityManager {#1298 …11}
    -backRefFieldName: "user"
    -typeClass: ClassMetadata {#1623 …}
    -isDirty: false
    #collection: ArrayCollection {#3210 ▼
      -elements: []
    }
    #initialized: true
  }
  -settings: PersistentCollection {#8189 ▼
    -snapshot: []
    -owner: User {#3161}
    -association: array:15 [ …15]
    -em: EntityManager {#1298 …11}
    -backRefFieldName: "user"
    -typeClass: ClassMetadata {#1829 …}
    -isDirty: false
    #collection: ArrayCollection {#3165 ▼
      -elements: []
    }
    #initialized: true
  }
  -imageFile: null
  -imageName: null
  -imageSize: null
  -enabled: false
  -credential: EmailPassword {#8095 ▼
    -email: "test@test.com"
    -password: "$2y$13$PBozBPUemeKghIg8c3Q1sO6oj8MGi93zp5PqMf1x4ngPyuPAWb.F."
  }
  -passwordResetToken: null
  -passwordRequestedAt: null
  -confirmationToken: null
  -confirmedAt: null
  -lastUpdatedAt: null
}

As you can see the id of user after flush is set to 33. The problem is that in the database the id of new user is 32.

The config\packages\msgphp_user.yaml file:

msgphp_user:
    class_mapping:
        MsgPhp\User\Entity\User: App\Entity\User
    default_id_type: integer

The xml with doctrine mapping:

<entity name="App\Entity\User" table="tbl_users" repository-class="App\Repository\UserRepository">

    <id name="id" column="id_users" type="integer">
        <generator strategy="IDENTITY"/>
    </id>

    <field name="firstName" column="first_name" type="string" length="50" nullable="true"/>
    <field name="lastName" column="last_name" type="string" length="50" nullable="true"/>
    <field name="abbreviation" column="abbreviation" type="string" length="3" nullable="true"/>
    <field name="birthDate" column="birth_date" type="date" nullable="true"/>
    <field name="phoneExtension" column="phone_extension" type="string" length="10" nullable="true"/>
    <field name="imageName" column="image_name" type="string" length="155" nullable="true"/>
    <field name="imageSize" column="image_size" type="integer" nullable="true"/>

    <many-to-one field="language" target-entity="App\Entity\Language" fetch="EAGER">
        <join-column name="lc" referenced-column-name="lc" />
    </many-to-one>

    <one-to-many field="roles" target-entity="App\Entity\UserRole" mapped-by="user" orphan-removal="true" fetch="EAGER">
        <cascade>
            <cascade-persist/>
            <cascade-remove/>
        </cascade>
    </one-to-many>

    <one-to-many field="settings" target-entity="App\Entity\UserSetting" mapped-by="user" orphan-removal="true">
        <cascade>
            <cascade-persist/>
            <cascade-remove/>
        </cascade>
    </one-to-many>
</entity>

Hope it helps. :)

ro0NL commented 6 years ago

Weird.. i'd say Doctrine/SQL at this point is responsible for the generated ID value =/

Could it be some sequence is corrupt or so on SQL's side?

Does the form mess things up maybe? i.e. the empty_data option causing a record in between.

Moreover, you dont really need data_class' => User::class,, IMHO it's simpler to do e.g.

if ($userForm->isSubmitted() && $userForm->isValid()) {
   $user = new User(new UserId(), $form->get('email')->getData(), ...);
}

nevertheless it should work i guess :)

crabnky commented 6 years ago

Thanks, this helped - I removed empty_data and data_class from the form and now it is working.

The nice feature of the previous solution was that I could use $user = $userForm->getData(); to load all date from the form - now I have to add each property manually - for example: $user->setFirstName($userForm->get('firstName')->getData());.

Anyway this is strange - and I'am sure it was working some time ago.

By the way, I found this good article: https://blog.martinhujer.cz/symfony-forms-with-request-objects/, so maybe this is the time to make some changes... :)

Thanks again.

ro0NL commented 6 years ago

Cool :) separating layers is good yes :+1:

Moreover, you could simplify things even further, using msgphp code ;)

// DomainObjectFactoryInterface $factory
$userId = $factory->identify(User::class, 1);
$user = $factory->create(User::class, ['constructor_arg' => 'value']);

Or

// DomainMessageBusInterface $bus
$bus->dispatch(new CreateUserCommand(['constructor_arg' => 'value']));

// mapping form fields against constructor args, and you're down to
$bus->dispatch(new CreateUserCommand($form->getData());

$bus->dispatch(new EnableUserCommand($userId));

The latter example assumes you know the user id upfront, or is queried separately.

Just showing you various flavors to do things ;)

Can we close? Should we investigate the form issue further, i.e. do we understand what happened :P

ro0NL commented 6 years ago

Also An entity should be always valid. should be the law :)

crabnky commented 6 years ago

To be honest I don't understand what is happening - now when adding new user with roles I'm getting error when saving roles:

An exception occurred while executing 'INSERT INTO tbl_users_userroles (id_users, id_userroles) VALUES (?, ?)' with params [36, 5]:

and the id of new user is... 37, so now the id used for query is DECREASED by one. WTF? LOL. :)

Not sure if this is related to your code - rather to doctrine or MSSQL server? Not sure, but I think we may close this issue.

ro0NL commented 6 years ago

My first guess it's related to MSSQL, messing up identities. Perhaps related to the IDENTITY definition? After reading https://www.databasejournal.com/features/mssql/article.php/3307541/Getting-the-Wrong-Identity-in-Microsoft-SQL-Server-identity-Columns.htm a bit :)

ro0NL commented 6 years ago

Closing for now. Feel free to re-open with more info :)