Add 'add_cap_letters' config option

[alllexx88]

This allows to add user-specified letters for serie/author grouping, e.g.: add_cap_letters = "АБВГДЕЁЖЗИЙКЛМНОПРСТУФХЦЧШЩЪЫЬЭЮЯ" to add Russian alphabet. This can be used to list any utf8 characters

[alllexx88]

Since this PR had a simple merge conflict in Config.php and config.ini.sample, I made a forced push on my branch to have a clean PR, on top of your changes, instead of resolving merge conflicts :)

[alllexx88]

Ah, I didn't update tests, that's why travis complains :)

I had to add '$app' parameter to author and series countGroupedByFirstLetter(), and tests don't know that :)

[alllexx88]

But I've tested it manually to work perfectly fine in adding any utf8 characters to grouping characters, like Japanese '唐宋音' Kanji :)

[alllexx88]

OK, I really don't know why, but with MySQL this works perfectly, while with SQLite, it works fine with "Access by Series", but not with "Access by Author": in the latter case all additional letters stay grayed out, and '#' has all authors starting with those 'additional' letters... It's driving me nuts, 'cause: a) I don't see anything mysql-specific in my PR b) I don't see any meaningful difference between handling authors and series in my PR

I hope you can help me figuring it out :(

[mduplouy]

Please do not inject the whole DIC as it's a bad thing (objects dependencies cannot be identified)

the DIC is injected into controllers by silex and as long as i don't make them services, i won't have the choice, that the only part where the DIC is fully injected

you should rather add only the array of letters from the config object so it can have a default value of empty array and this won't break the tests

for now you must extend the string utils service after the config is initialized (as it's used by config, otherwise you will get infinite loop), there you can set the letters from config into the string utils object

i've already planned to rework that part, it's too much complicated

[alllexx88]

@mduplouy Right, I get it. Is it OK now? But the mysterious issue I've mentioned is still there: it's OK with mysql, but author grouping still doesn't work as expected with sqlite...

[alllexx88]

I fixed SeriesCollectionTest, but no idea about these failures:

There were 2 failures: 1) Cops\Tests\Front\Controller\OpdsControllerTest::testUrl with data set #1 ('/test/fr/opds/authors') Failed asserting that false is true. /home/travis/build/mduplouy/silex-cops/tests/Unit/Cops/Tests/Front/Controller/OpdsControllerTest.php:16 2) Cops\Tests\Front\Controller\OpdsControllerTest::testUrl with data set #4 ('/test/fr/opds/series') Failed asserting that false is true. /home/travis/build/mduplouy/silex-cops/tests/Unit/Cops/Tests/Front/Controller/OpdsControllerTest.php:16

It's too non-informative for me :(

[mduplouy]

do not bother modifying the opds, it's not working atm

i will rework this upon a view model pattern :)

[alllexx88]

ok i think it's a bit too intrusive regarding the preg_split, i will think about a way to handle the latin letters pattern globally rather than duplicate it in controllers (duplication is evil is one of my motto)

I think we need a uniform way to provide access to both a string, and an array of all letters: latin, and additional, to whichever instances that require them. This much duplication indeed makes it hard to maintain/modify code. It's OK to use any utf8 characters as URL for twig, even in old PHP: it transforms them to HTML entities, we just need to remember to do html_entity_decode() later. Until I realized this, assert('letter', '\w+|0|['.($addlettersstr).']') was silently failing on home page when clicking on non-latin letters, and lists not being constructed, and attempts to access respective URLs directly gave url resolver error. Beyond that, it's just a matter to simplify the way of handling/providing letters string/array.

And yet another thought on letter checking regex'es. What if some special character (like ]) is added to the list? We need to escape them. So in fact we should better provide a ready, properly escaped regex, instead of a letters string. Same goes for array: what if ' is given? Will we have an SQL syntax error then? (Don't know -- suspect doctrine handles this properly)

But in general, does the idea of this PR look right?

Thanks :)

[mduplouy]

Yes the idea to provide full or part of the grouping letter is fine, it's just the way it' made now that is not clean enough for me :)

there is surely a way to make it without having to check all along the controllers, but i need to think about it (middleware, event dispatcher..)

[alllexx88]

@mduplouy agree, I don't like it either -- too much room for mistake :) That's actually because latin letters are hardcoded in too many places, and I'm afraid I'm not knowledgeable enough in both your code, and silex-cops operation in general, to make this change nice and clean. If you can do this, it'd be wonderful: this PR is fun, and I learned a lot, but my expertize lies elsewhere, you're obviously better at this :D