Ignore tag's case #3139
Ignore tag's case #3139
Conversation
|
Are tests on MySQL green locally? |
|
@j0k3r will check next week |
|
Well... After checking the failing test I think I'm hitting a case sensitivity issue with databases again (thanks MySQL for being case-insensitive). I will rework the PR. |
|
I updated the test for WallabagV1Import. Please review carefully @j0k3r @nicosomb @tcitworld Reminder: this PR removes the ability to have upper chars in tag labels. |
|
Do we need a migration to update current tags? |
| * @param array $entitiesReady Entities from the EntityManager which are persisted but not yet flushed | ||
| * It is mostly to fix duplicate tag on import @see http://stackoverflow.com/a/7879164/569101 | ||
| */ | ||
| public function assignTagsToEntry(Entry $entry, $tags, array $entitiesReady = []) |
There was a problem hiding this comment.
Hum, in 2.3 assignTagsToEntry has moved to Wallabag\CoreBundle\Helper\TagsAssigner.
Please update the correct one 🙂
There was a problem hiding this comment.
Well I blindly rebased my work without seeing that the whole method was missing...
|
I need to add a specific test case for checking backward compatibility. |
|
Go ahead 🙂 |
|
I broke something really hard while trying to fix some tests. It's way more
complicated than I initially thought when dealing with same tags with
different cases. The better way will be to migrate all previous tags in
lowercase. I'll handle that on the next week.
Sorry for the delay :(
…Sent from my phone
|
|
I am going to resume work on this PR this evening. |
|
The migration of tags is mostly done but I found another issue regarding unicode characters: PHP function Should we improve the support of unicode chars on the app side or should we just convert label like slug instead? |
|
Well... Correction: only SQLite has issues with LOWER-ing unicode characters. |
|
Please check the message of the commit introducing migration for detail. Note also that SQLite is unsupported by this migration. Extensive test is required for migrating existing databases on MySQL and PostgreSQL. |
| ); | ||
|
|
||
| // Delete unused tags | ||
| $this->addSql(" |
There was a problem hiding this comment.
What if the tag is associated to some entries?
We should handle the relation table entry_tag too 😕
There was a problem hiding this comment.
This is the goal of the SQL block just above it
|
Could you run a php-cs-fixer? And I think we'll be good to go 🙂 |
Fixes #2502 Signed-off-by: Kevin Decherf <kevin@kdecherf.com>
This migration does not support SQLite as long as this engine does not support Unicode in LOWER(). This migration starts by retrieving the list of lowercase tags which need to be migrated. Then it retrieves the list of tags for each tags from the previous step in order to migrate entries. It handles deletion of empty tags. At the end the migration makes a full scan to update the label of all remaining tags. WARNING: THIS MIGRATION IS IRREVERSIBLE. Signed-off-by: Kevin Decherf <kevin@kdecherf.com>
Signed-off-by: Kevin Decherf <kevin@kdecherf.com>
|
👏 |
Fix possible issue with special chars on #3139 Signed-off-by: Kevin Decherf <kevin@kdecherf.com>
Fix possible issue with special chars on #3139 Signed-off-by: Kevin Decherf <kevin@kdecherf.com>
Store tag labels in lowercase in order to prevent case-sensitive duplicates.
Non-lowercase tags already existing before this PR should still work as usual but any new tag will be saved in lowercase.