Skip to content
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.

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

[Translatable] Add unique index name generator ( use doctrine method), and support different column name in PK. #348

Open
wants to merge 7 commits into
base: master
from

Conversation

@eisberg
Copy link

commented Feb 7, 2017

Translatable: Add unique index name generator ( use doctrine method), and support different column name in PK.

Fix issues:
Translatable : Identifier name is too long #178
Translatable, is field "id" required in translatable class? #335

Example length of autogenerate name: map_type_translations_unique_translation - 40chars
Oracle database object names maximum length is 30 bytes. Object Name Rules

If the length of this uniqueIndex more than 30 characters - it will create a unique name using the algorithm of the doctrine, the number 30 - may vary depending on the platform and is also taken from the doctrine.

Also, problem with hardcoding primary key name in the code names is solved.
The default name: id

Example Translation Entity:

/**
 * @ORM\Table(name="MAP_TYPE_TRANSLATIONS" , uniqueConstraints={@ORM\UniqueConstraint(name="map_type_translations_unique_translation",columns={"TYPE_ID", "LOCALE"})})
 * })
 * @ORM\Entity()
 * @ORM\HasLifecycleCallbacks()
 *
 */
class MapTypeTranslation  implements JsonSerializable
{
    use ORMBehaviors\Translatable\Translation;

    /**
     * @var MapType
     *
     * @ORM\ManyToOne(targetEntity="MapType", fetch="LAZY")
     * @ORM\JoinColumns({
     *   @ORM\JoinColumn(name="TYPE_ID", referencedColumnName="TYPE_ID", onDelete="CASCADE", unique=true)
     * })
     */
    protected $translatable;

wucdbm and others added some commits Sep 28, 2016

Translatable: Add unique index name generator ( use doctrine method),…
… and support different column name in PK.
@Pierstoval
Copy link

left a comment

Lots of code sniffing issues, and bad practice in gitignore and composer.json

.gitignore Outdated
@@ -2,3 +2,4 @@ composer.phar
vendor
composer.lock
bin
.idea

This comment has been minimized.

Copy link
@Pierstoval

Pierstoval Feb 9, 2017

This should not be in project's gitignore but in your own global gitignore file

@@ -1,5 +1,5 @@
{
"name": "knplabs/doctrine-behaviors",
"name": "modeltech/doctrine-behaviors",

This comment has been minimized.

Copy link
@Pierstoval

Pierstoval Feb 9, 2017

You should not change the vendor name on the main repo

@@ -56,7 +63,7 @@ services:
knp.doctrine_behaviors.translatable_subscriber.default_locale_callable:
class: "%knp.doctrine_behaviors.translatable_subscriber.default_locale_callable.class%"
arguments:
- "%locale%"
- %locale%

This comment has been minimized.

Copy link
@Pierstoval

Pierstoval Feb 9, 2017

Parameters should still be wrapped under quotes for YAML best practices and to avoid deprecations

}
}

This comment has been minimized.

Copy link
@Pierstoval

Pierstoval Feb 9, 2017

You should add a newline at the end of the file and fix code styling (spaces, new lines, etc.)

private $currentLocaleCallable;
private $defaultLocaleCallable;
private $translatableTrait;
private $translationTrait;
private $translatableFetchMode;
private $translationFetchMode;
public function __construct(ClassAnalyzer $classAnalyzer, callable $currentLocaleCallable = null,
public function __construct(ClassAnalyzer $classAnalyzer, UniqueIndexNameGeneratorInterface $uniqueIndexNameGenerator, $isRecursive, callable $currentLocaleCallable = null,

This comment has been minimized.

Copy link
@Pierstoval

Pierstoval Feb 9, 2017

Can you chop down all arguments so the constructor is more clear about the different arguments?

@@ -73,7 +76,9 @@ public function loadClassMetadata(LoadClassMetadataEventArgs $eventArgs)
}
if ($this->isTranslation($classMetadata)) {
$this->mapTranslation($classMetadata);

This comment has been minimized.

Copy link
@Pierstoval

Pierstoval Feb 9, 2017

Too much newlines here

$translatableColumnName = 'translatable_id';

This comment has been minimized.

Copy link
@Pierstoval

Pierstoval Feb 9, 2017

Same here, fix newlines

public function generate( $columnNames, $prefix='', $maxSize=30 );
}

This comment has been minimized.

Copy link
@Pierstoval

Pierstoval Feb 9, 2017

Fix newlines here too

@eisberg

This comment has been minimized.

Copy link
Author

commented Feb 9, 2017

@Pierstoval, i fixed it

@Pierstoval
Copy link

left a comment

One line feed and it's ok for cs issues :)

.gitignore Outdated
@@ -1,4 +1,4 @@
composer.phar
vendor
composer.lock
bin
bin

This comment has been minimized.

Copy link
@Pierstoval

Pierstoval Feb 9, 2017

Still need a newline at the end of the file here

This comment has been minimized.

Copy link
@eisberg

eisberg Feb 9, 2017

Author

:) Ok, added

alexfrg added some commits Feb 9, 2017

Change private to public functions: getDefaultLocale, getCurrentLocal…
…e example access from repository or walker

@Einenlum Einenlum added enhancement bug and removed enhancement labels Sep 14, 2017

@Einenlum Einenlum changed the title Translatable: Add unique index name generator ( use doctrine method), and support different column name in PK. [Translatable] Add unique index name generator ( use doctrine method), and support different column name in PK. Sep 14, 2017

@treatys

This comment has been minimized.

Copy link

commented Aug 5, 2019

Do you intend to release it? I am just wondering why I do need to specify unique index in Translation class, while it's obviously ID of Translatable class + language.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.