8000 Bug Report: msgphp/user: respect mysql/mariadb max length for indexes/unique · Issue #210 · msgphp/msgphp · GitHub
[go: up one dir, main page]
Skip to content

Bug Report: msgphp/user: respect mysql/mariadb max length for indexes/unique #210

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.

Sign up for GitHub

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

Jump to bottom
Closed
pascalwacker opened this issue Oct 24, 2018 · 20 comments
Closed

Bug Report: msgphp/user: respect mysql/mariadb max length for indexes/unique #210

pascalwacker opened this issue Oct 24, 2018 · 20 comments
Labels
bundle feature infrastructure

Comments

@pascalwacker
Copy link
Contributor

Bug Report: I've just tried to install the user bundle: https://github.com/msgphp/user which went well until I tried to run the db migrations.

Steps to reproduce, take a Symfony 4.1 web skeleton, run composer require annot form validator twig security messenger orm, composer require maker --dev, composer require msgphp/user-bundle and bin/console make:user:msgphp

I've answered all questions with the default value and approved all writes. After that I ran bin/console make:migration and bin/console doctrine:migrations:migrate

The code generated by bin/console make:migration looked like this:

$this->addSql('CREATE TABLE user_role (user_id INT NOT NULL COMMENT \'(DC2Type:msgphp_user_id)\', role_name VARCHAR(255) NOT NULL, INDEX IDX_2DE8C6A3A76ED395 (user_id), INDEX IDX_2DE8C6A3E09C0C92 (role_name), PRIMARY KEY(user_id, role_name)) DEFAULT CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci ENGINE = InnoDB');
$this->addSql('CREATE TABLE role (name VARCHAR(255) NOT NULL, PRIMARY KEY(name)) DEFAULT CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci ENGINE = InnoDB');
$this->addSql('CREATE TABLE user (id INT AUTO_INCREMENT NOT NULL COMMENT \'(DC2Type:msgphp_user_id)\', credential_email VARCHAR(255) NOT NULL, credential_password VARCHAR(255) NOT NULL, password_reset_token VARCHAR(255) DEFAULT NULL, password_requested_at DATETIME DEFAULT NULL, UNIQUE INDEX UNIQ_8D93D649A5D24B55 (credential_email), UNIQUE INDEX UNIQ_8D93D6496B7BA4B6 (password_reset_token), PRIMARY KEY(id)) DEFAULT CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci ENGINE = InnoDB');
$this->addSql('ALTER TABLE user_role ADD CONSTRAINT FK_2DE8C6A3A76ED395 FOREIGN KEY (user_id) REFERENCES user (id) ON DELETE CASCADE');
$this->addSql('ALTER TABLE user_role ADD CONSTRAINT FK_2DE8C6A3E09C0C92 FOREIGN KEY (role_name) REFERENCES role (name) ON DELETE CASCADE');

When I ran the migration file using bin/console doctrine:migrations:migrate I got an error, telling me, that primary key and unique column where to long:

Migration 20181024154539 failed during Execution. Error An exception occurred while executing 'CREATE TABLE role (name VARCHAR(255) NOT NULL, PRIMARY KEY(name)) DEFAULT CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci ENGINE = InnoDB':

SQLSTATE[42000]: Syntax error or access violation: 1071 Specified key was too long; max key length is 767 bytes

I "fixed" (more a temporary hack ;)) it by adding a length=25 inside the @ORM\Column() statement for the name in \App\Entity\User\Role and changed the migration file to only have 90 characters for credential_email and password_reset_token in the user table.

It would be great if either the bundle could respect mysql/mariadb max index size or to have a hint in the bundles README.md file (and also how to restrict the character amount of the fields).
@ro0NL
Copy link
Contributor
ro0NL commented Oct 24, 2018

Hi @pascalwacker ,

I think it's a recall for #159 :) however, im still not sure how to tackle this from a vendor perspective. The attributeOverride option looks the cleanest to me =/

I'll try to give it some more thought.

@pascalwacker
Copy link
Contributor Author
pascalwacker commented Oct 24, 2018 •
edited
8000 Loading

@ro0NL so I tried to add an override, unfortunately it told me, there's no attribute email. Since the email is supplied by the credential. What would I need to do to apply an override to the email?

Error: Invalid field override named 'email' for class 'App\Entity\User\User'.

The used override:

/**
 * @ORM\Entity()
 * @ORM\AttributeOverrides({
 *      @ORM\AttributeOverride(name="email",
 *          column=@ORM\Column(
 *              name     = "credential_email",
 *              length   = 191,
 *              unique   = true
 *          )
 *      ),
 *      @ORM\AttributeOverride(name="passwordResetToken",
 *          column=@ORM\Column(
 *              name     = "password_reset_token",
 *              length   = 191,
 *              unique   = true
 *          )
 *      )
 * })
 */
class User extends BaseUser implements DomainEventHandlerInterface
{ ... }

@ro0NL
Copy link
Contributor
ro0NL commented Oct 24, 2018 •
edited
Loading

i never really tested it :)

The field is derived from an credential embeddable (User::$credential), so perhaps try credential.email, but im not sure you can override embeddable fields at the entity level (that would be desired though).

Otherwise, and worst case, you'll need to override the Credential class and override it's own field from there on. Or, we do something clever in core, but i'm not sure yet. In theory we should be able to generate specific mapping files, as those are copied from dist files.

@pascalwacker
Copy link
Contributor Author

Unfortunately credential.email also doesn't work. I also tried to only override the passwordResetToken, which also threw the error: Invalid field override named 'passwordResetToken' for class 'App\Entity\User\User'., I'm therefore not sure if embedded fields can be overriden in doctrine.

About overriding the Credential class: Am I guessing right, that I would have to override \MsgPhp\User\Entity\Credential\Features\EmailAsUsername? However, how would I add there a restriction, as the private $email; isn't a ORM\Column()?

@pascalwacker
Copy link
Contributor Author

It seams to be the case, that embedded properties can't be overridden yet: doctrine/orm#6047 but then on the other hand, the doctrine doc says it should work: https://www.doctrine-project.org/projects/doctrine-orm/en/2.6/tutorials/override-field-association-mappings-in-subclasses.html#override-field-association-mappings-in-subclasses

@ro0NL
Copy link
Contributor
ro0NL commented Oct 24, 2018

You can copy https://github.com/msgphp/msgphp/blob/master/src/User/Entity/Credential/EmailPassword.php and add the ORM annotations as needed.

Next, copy the trait for your User entiy: https://github.com/msgphp/msgphp/blob/master/src/User/Entity/Features/EmailPasswordCredential.php where you wire your custom credential. Last, update your User constructor to initialize your custom credential instead.

That's the theory. Actually documenting this is still on my lists. So if it works out, please share. But ideally i like to handle this in core somehow.

@ro0NL
Copy link
Contributor
ro0NL commented Oct 24, 2018

For the reset password feature, copy https://github.com/msgphp/msgphp/blob/master/src/User/Entity/Features/ResettablePassword.php

and explicitly enable its domain event:

msgphp_user:
  commands:
    MsgPhp\User\Command\RequestUserPasswordCommand: true

But i agree, this is a lot of work only to override a ORM mapping :(

@pascalwacker
Copy link
Contributor Author

What about the file in vendor/msgphp/user/Infra/Doctrine/Resources/dist-mapping/User.Entity.Credential.EmailPassword.orm.xml can I somehow override that? As far as I can see, it get's loaded at \MsgPhp\UserBundle\DependencyInjection\Extension::getDoctrineMappingFiles, using the dir from \MsgPhp\UserBundle\DependencyInjection\Configuration::getPackageDir

If I could just override the .orm.xml files, I guess my problems would be solved.

@ro0NL
Copy link
Contributor
ro0NL commented Oct 24, 2018

they are copied as-is to the cache folder, so not really.

But i think this is actually a nice way to customize: being able to own/modify the mapping files.

Something like

msgphp_user:
  doctrine:
    mapping: { dir: 'some/path/', type: xml }

Anything not defined there will be inherited from the core dist files.

@pascalwacker
Copy link
Contributor Author

How about copying them to config/packages/msgphp_user/mappings/[name].orm.xml and load them from there, this way it would be clear where to find them?

Btw. I tried to use a service decorator,

    App\Decorators\MsgPhp\UserBundle\DependencyInjection\Extension:
        decorates: MsgPhp\UserBundle\DependencyInjection\Extension

To override the Extension class, and was planing on forwarding all the calls to the original one, except for the prepend function, unfortunately it threw my an ServiceNotFoundException for MsgPhp\UserBundle\DependencyInjection\Extension do you know if any of the classes involved is defined/called as a service, that I could decorate in my services.yaml file?

@ro0NL
Copy link
Contributor
ro0NL commented Oct 24, 2018 •
edited
Loading

Extension class is a corner stone of the bundle, and not meant to be used as a service. You can't go down that road.

For now your best hook-in point is https://github.com/msgphp/msgphp/blob/master/src/Domain/Infra/Doctrine/MappingCacheWarmer.php which is registered as a service and copies the mapping files to the cache folder.

The reason we do this, is some mapping files are optional (e.g. UserAttributeValue.orm.xml which requires msgphp/eav) so the results are a bit dynamic (depending on which features are enabled). That's the reason i chosen the cache folder, to hide this detail from the user.

But i like the idea to detect overrides in config/packages/msgphp_user/mappings/[name].orm.xml one way or another yes. Ill have a look :)

@pascalwacker
Copy link
Contributor Author

So what I ended up doing was decorating the MappingCacheWarmer:

// services.yaml

[...]
    App\Decorators\MsgPhp\Infra\Doctrine\MappingCacheWarmer:
        decorates: MsgPhp\Domain\Infra\Doctrine\MappingCacheWarmer
        public: false
        arguments:
            - 'msgphp/doctrine-mapping'
            - '%msgphp.doctrine.mapping_files%'
            - '%kernel.project_dir%'
        tags:
            - { name: 'kernel.cache_warmer', priority: 100 }

<?php
// App\Decorators\MsgPhp\Infra\Doctrine\MappingCacheWarmer

declare(strict_types=1);

namespace App\Decorators\MsgPhp\Infra\Doctrine;

use Symfony\Component\Filesystem\Filesystem;
use Symfony\Component\HttpKernel\CacheWarmer\CacheWarmerInterface;

final class MappingCacheWarmer implements CacheWarmerInterface
{
    private $cacheWarmer;
    private $dirName;
    private $mappingFiles;
    private $rootDir;

    public function __construct(string $dirName, array $mappingFiles, string $rootDir, \MsgPhp\Domain\Infra\Doctrine\MappingCacheWarmer $cacheWarmer)
    {
        $this->cacheWarmer = $cacheWarmer;
        $this->dirName = $dirName;
        $this->mappingFiles = $mappingFiles;
        $this->rootDir = $rootDir;
    }

    public function isOptional(): bool
    {
        return $this->cacheWarmer->isOptional();
    }

    public function warmUp($cacheDir)
    {
        // let default warmup happen
        $this->cacheWarmer->warmUp($cacheDir);

        $filesystem = new Filesystem();
        foreach ($this->mappingFiles as $mappingFile) {
            $fileName = basename($mappingFile);

            if ($filesystem->exists($this->rootDir . '/config/packages/msgphp/user/' . $fileName)) {
                $filesystem->copy($this->rootDir . '/config/packages/msgphp/user/' . $fileName, $cacheDir.'/'.$this->dirName.'/'.$fileName, true);
            }
        }
    }
}

I then copied the file in vendor/msgphp/user/Infra/Doctrine/Resources/dist-mapping/User.Entity.Credential.EmailPassword.orm.xml to config/packages/msgphp/user/User.Entity.Credential.EmailPassword.orm.xml and modified it (adding a length="191" to it)

<?xml version="1.0" encoding="UTF-8"?>
<doctrine-mapping xmlns="http://doctrine-project.org/schemas/orm/doctrine-mapping"
    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
    xsi:schemaLocation="http://doctrine-project.org/schemas/orm/doctrine-mapping http://doctrine-project.org/schemas/orm/doctrine-mapping.xsd">

    <embeddable name="MsgPhp\User\Entity\Credential\EmailPassword">
        <field name="email" unique="true" length="191" />
        <field name="password" />
    </embeddable>

</doctrine-mapping>

@ro0NL
Copy link
Contributor
ro0NL commented Oct 24, 2018 •
edited
Loading

👏 so let's simplify from here on, if the overridden files end up in the core cache warmer we should be good. Does the proposed config work for you? In your case it be:

msgphp_user:
  doctrine:
    mapping: '%kernel.project_dir%/config/packages/msgphp/user'

That can be used from here on:

msgphp/src/UserBundle/DependencyInjection/Extension.php

Line 119 in 74c7166

private static function getDoctrineMappingFiles(array $config, ContainerBuilder $container): array

@pascalwacker pascalwacker mentioned this issue Oct 24, 2018
Closed
@pascalwacker
Copy link
Contributor Author

@ro0NL I've created a pull request (#211) with a possible fix. I personally would prefer to keep the default config files but be able to override some of them.
8000

@pascalwacker
Copy link
Contributor Author

Seams there's still one problem, which the pull doesn't solve (yet). The ObjectFieldMapping class in the User namespace defines a ResettablePassword feature: https://github.com/msgphp/msgphp/blob/master/src/User/Infra/Doctrine/ObjectFieldMappings.php#L42 which is unique. Any idea how to solve this?

@ro0NL
Copy link
Contributor
ro0NL commented Oct 25, 2018 •
edited
Loading

@pascalwacker for now i think you can redeclare the trait properties in your class, e.g.

msgphp/src/User/Entity/Features/ResettablePassword.php

Line 15 in 74c7166

private $passwordResetToken;

and add the annotation while doing so. That should override the default mapping here.

We might need another feature to override the built-in trait/object mappings more globally :P

@ro0NL
Copy link
Contributor
ro0NL commented Oct 25, 2018

Actually you can create your own ObjectFieldMappingsProviderInterface service and override existing / provide new mappings. Not documented.. sorry :)

@ro0NL ro0NL mentioned this issue Oct 25, 2018
Closed
@pascalwacker pascalwacker mentioned this issue Oct 25, 2018
Closed
@pascalwacker
Copy link
Contributor Author

Currently there are 3 open pull requests, aiming at solving this issue

My suggestion would be to use pull 214 and 211. Since both modify the `CacheWarmer, they can't simply be merged. If you agree on this path, I would modify pull 214 to incorporate the changes from 211, this way we would get a nice config parameter for key lengths that's respected by every thing, as well as the ability to override selected config files, while keeping the default ones.

Let me know what you think.

@ro0NL
Copy link
Contributor
ro0NL commented Oct 25, 2018

@pascalwacker i like to continue with #214 #211 indeed 👍

From #214 if we have a MappingConfig it can be leveraged by #211 as well, e.g. MappingConfig::$mappingDir

@pascalwacker
Copy link
Contributor Author
pascalwacker commented Oct 25, 2018 •
edited
Loading

@ro0NL I've modified #214 to incorporate the changes from #211

From my point of view, pull requests 211 and 213 can be closed and 214 be merged, as it incorporates all changes from 211 and 213.

@ro0NL ro0NL closed this as completed Oct 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bundle feature infrastructure
Milestone
No milestone
Development

No branches or pull requests
2 participants
@pascalwacker @ro0NL
0