Tag Module converted to Doctrine #1156
Conversation
get, getForItem, getForMultipleItems
…:getRelatedItemsByTag
| * | ||
| * @ORM\Column(type="string", length=255) | ||
| */ | ||
| private $tag; |
There was a problem hiding this comment.
I would use 'name' and not 'tag', since you will store the 'name' property of the 'tag' object in here. This will make it less confusing I guess.
Since this breaks other modules (like f.e.: blog) which rely on this.
This is more descriptive.
Also integrated modules_tags in the Tags module installer
| @@ -1,8 +1,15 @@ | |||
| CREATE TABLE IF NOT EXISTS `tags` ( | |||
There was a problem hiding this comment.
this file should be completely removed in favor of installing doctrine entities (see https://github.com/forkcms/forkcms/blob/doctrine/src/Backend/Modules/ContentBlocks/Installer/Installer.php#L34-L36)
|
Isn't this a great opportunity to rename all the |
|
@turanct That's a good idea. What do you think @WouterSioen |
|
Poke @WouterSioen |
|
I'm in Iceland at the moment, I'll be back the 20th of april. Op maandag 13 april 2015 heeft Jeroen Desloovere notifications@github.com
|
|
I'm 100% in favour of good variable names that says what the variable contains. renaming stuff like item, value, record,... to something more meaningful would thus be a great thing to do! |
|
@turanct @WouterSioen Done. |
|
@WouterSioen What seems to be the Scrutinizer Error? |
|
The doctrine branch does not contain tests yet, so no code coverage could be generated. Don't worry about it ;) |
Tag Module converted to Doctrine
|
@WouterSioen Is this branch ready to be merged with the Doctrine branch? Or do I need to change something? |
|
it's merged :) |
|
@WouterSioen thanks |
Replaced
databaseforDoctrinein Backend and Frontend Tags Module.