added alias field, callback and tags_title inserttag #6

Defcon0 · 2017-09-04T09:24:28Z

we just added an alias field to your tags_bundle and would very much appreciate it if you could merge the commit into the main project. The commit in detail contains:

an alias field
the necessary callbacks
the global select option for batch generating aliases
localization for English
an insert tag called "tags_title" that outputs a tag's title by inserting a source name and the id or the alias of a tag

We also added the translation for German for all existing strings.

coveralls · 2017-09-04T09:26:54Z

Coverage decreased (-35.9%) to 64.078% when pulling 0638264 on heimrichhannot:alias into 09e31d9 on codefog:master.

Defcon0 · 2017-09-04T09:37:03Z

@qzminski Is there something I can do about the coveralls issue?

coveralls · 2017-09-04T10:05:44Z

Coverage decreased (-35.9%) to 64.078% when pulling 5b2aeaf on heimrichhannot:alias into 09e31d9 on codefog:master.

…efaultManager

coveralls · 2017-09-04T14:09:54Z

Coverage decreased (-35.9%) to 64.078% when pulling e71156c on heimrichhannot:alias into 09e31d9 on codefog:master.

qzminski

In addition to comments in the review please provide unit tests that will keep the code coverage at the same level and apply the code style (php-cs-fixer is a dev requirement already).

qzminski · 2017-09-05T11:32:12Z

composer.json

@@ -1,5 +1,5 @@
 {
-    "name": "codefog/tags-bundle",
+    "name": "heimrichhannot/tags-bundle",


qzminski · 2017-09-05T11:33:27Z

src/EventListener/DataContainer/TagListener.php

+     *
+     * @throws \Exception
+     */
+    public function generateAlias($value, DataContainer $dc)


This whole method needs to be reworked to use strict types and comparisons, database service, etc.

qzminski · 2017-09-05T11:33:45Z

src/EventListener/DataContainer/TagListener.php

+     *
+     * @return array
+     */
+    public function addAliasButton($buttons, \DataContainer $dc)


This whole method needs to be reworked to use strict types and comparisons, database service, etc.

qzminski · 2017-09-05T11:34:27Z

src/EventListener/InsertTagsListener.php

+        switch ($insertTag) {
+            case 'tags_title':
+                return $tags->first()->name;
+                break;


This break statement is not needed here.

qzminski · 2017-09-05T11:36:08Z

src/EventListener/InsertTagsListener.php

+                break;
+        }
+
+        return '';


I think we should throw an exception here and not return an empty string.

qzminski · 2017-09-05T11:36:58Z

src/Manager/DefaultManager.php

@@ -190,7 +190,7 @@ private function createTag(string $value): Tag
     *
     * @return array
     */
-    private function getCriteria(array $criteria = []): array
+    protected function getCriteria(array $criteria = []): array


What is the reason you changed the properties and methods visibility?

We adjusted it so that inheriting services can use the attributes and methods themselves without needing to redefine them.

qzminski · 2017-09-05T11:37:54Z

src/Model/TagModel.php

+                throw new NoTagsException();
+            }
+
+            $columns[] = 'alias IN ('.implode(',', array_map(function($alias) {


Just use "alias IN ('" . implode("','", $criteria['aliases']) . "')".

qzminski · 2017-09-05T11:38:48Z

src/Resources/contao/dca/tl_cfg_tag.php

+            'inputType'     => 'text',
+            'eval'          => ['rgxp' => 'alias', 'unique' => true, 'maxlength' => 128, 'tl_class' => 'w50'],
+            'save_callback' => [['codefog_tags.listener.data_container.tag', 'generateAlias']],
+            'sql'           => "varchar(128) COLLATE utf8_bin NOT NULL default ''",


Please adjust the SQL format just like it is for the other fields.

Of course, I can do that, but at the moment I have no idea how to define the collation:

I tried 'collation' => 'utf8_bin' and 'collate' => 'utf8_bin'. None of them worked out :-(

coveralls · 2017-09-07T07:03:36Z

Coverage decreased (-35.9%) to 64.078% when pulling 7228b6f on heimrichhannot:alias into 09e31d9 on codefog:master.

coveralls · 2017-09-07T12:48:08Z

Coverage decreased (-36.2%) to 63.768% when pulling c3adb7f on heimrichhannot:alias into 09e31d9 on codefog:master.

coveralls · 2017-09-07T12:50:38Z

Coverage decreased (-29.8%) to 70.192% when pulling 5755bc9 on heimrichhannot:alias into 09e31d9 on codefog:master.

coveralls · 2017-09-07T12:51:31Z

Coverage decreased (-29.8%) to 70.192% when pulling 5755bc9 on heimrichhannot:alias into 09e31d9 on codefog:master.

Defcon0 · 2017-09-07T14:09:08Z

Still working on the tests. I'll write again after finishing the work.

coveralls · 2017-09-07T14:16:52Z

Coverage decreased (-29.8%) to 70.192% when pulling 4e52293 on heimrichhannot:alias into 09e31d9 on codefog:master.

coveralls · 2017-09-07T14:35:10Z

Coverage decreased (-29.3%) to 70.673% when pulling 7d6b636 on heimrichhannot:alias into 09e31d9 on codefog:master.

coveralls · 2017-09-07T15:01:01Z

Coverage decreased (-28.8%) to 71.154% when pulling bb113f6 on heimrichhannot:alias into 09e31d9 on codefog:master.

coveralls · 2017-09-07T15:05:06Z

Coverage decreased (-20.3%) to 79.67% when pulling 16297a1 on heimrichhannot:alias into 09e31d9 on codefog:master.

coveralls · 2017-09-08T09:36:06Z

Coverage decreased (-1.8%) to 98.182% when pulling ef72db2 on heimrichhannot:alias into 09e31d9 on codefog:master.

coveralls · 2017-09-08T09:56:31Z

Coverage decreased (-1.2%) to 98.788% when pulling ad86446 on heimrichhannot:alias into 09e31d9 on codefog:master.

Defcon0 · 2017-09-08T10:07:52Z

OK, I'm done with the tests. Some thoughts on them:

I didn't do testing that much, escpecially not for Contao 4/Symfony, so it might not be perfect ;-)
The last code coverage issues are a bit obscure to me:

in /src/EventListener/InsertTagsListener.php on

line 89: When debugging the test case in my local phpStorm, the debugger jumps at this line, so it indeed is covered. Might be an issue in coveralls??

line 109: How can I cover this line if it's logically never reached? An unsupported tag is filtered out beforehands. So the interpreter can't reach this line at all.

I ignored "addAliasButton" in TagListener.php since the function contains database, session and version handling. All of this taken from Contao Core and they don't test it either since the code isn't very testable at all. In addition the effort for doing so is much too high in my opinion. If you have any hint for me how to test it in a pragmatic way, you're very welcome ;-)

So is the request now ready for pulling?

qzminski · 2017-09-12T12:22:29Z

I guess I will have to tweak this PR a little bit and write the tests in a correct way but in general looks good. Because of the changes I have to make this will be merged in the upcoming weeks. Thanks @Defcon0 !

Defcon0 · 2017-09-12T12:25:14Z

@qzminski Great to hear that :-) I'll review the tweaked tests to do it better next time ;-)

qzminski · 2017-10-24T15:27:05Z

Hey @Defcon0 some of the features have been already implemented on the develop branch. It was just easier for me to pick up the necessary code/concept chunks and put individually there instead of merging this big PR, hope that's fine 😉 I am still about to add the insert tags listener there, so please be patient!

added alias field, callback and tags_title inserttag

0638264

fixed composer.json

5b2aeaf

change visibility of private methods and attributes to inherit from D…

e71156c

…efaultManager

qzminski requested changes Sep 5, 2017

View reviewed changes

fixed heimrichhannot namespace

7228b6f

Dennis Patzer added 2 commits September 7, 2017 14:45

codestyle and review fixed

85c4728

codestyle, latest changes

c3adb7f

removed contao test case inheritance

5755bc9

fixed composer.json

4e52293

added more test cases

7d6b636

fixed tests

bb113f6

fixed tests

16297a1

added tests

ef72db2

fixed codestyle, fixed tests

ad86446

qzminski added the enhancement label Sep 12, 2017

qzminski added a commit that referenced this pull request Oct 9, 2017

Added the German translation (#6)

bb75597

qzminski closed this Nov 9, 2017

fatcrobat deleted the alias branch November 23, 2017 09:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added alias field, callback and tags_title inserttag #6

added alias field, callback and tags_title inserttag #6

Defcon0 commented Sep 4, 2017

coveralls commented Sep 4, 2017

Defcon0 commented Sep 4, 2017

coveralls commented Sep 4, 2017

coveralls commented Sep 4, 2017

qzminski left a comment

qzminski Sep 5, 2017

Defcon0 Sep 7, 2017

qzminski Sep 5, 2017

qzminski Sep 5, 2017

qzminski Sep 5, 2017

qzminski Sep 5, 2017

qzminski Sep 5, 2017

Defcon0 Sep 7, 2017

qzminski Sep 5, 2017

qzminski Sep 5, 2017

Defcon0 Sep 7, 2017

coveralls commented Sep 7, 2017

coveralls commented Sep 7, 2017

coveralls commented Sep 7, 2017

coveralls commented Sep 7, 2017

Defcon0 commented Sep 7, 2017

coveralls commented Sep 7, 2017

coveralls commented Sep 7, 2017

coveralls commented Sep 7, 2017

coveralls commented Sep 7, 2017

coveralls commented Sep 8, 2017

coveralls commented Sep 8, 2017

Defcon0 commented Sep 8, 2017

qzminski commented Sep 12, 2017

Defcon0 commented Sep 12, 2017

qzminski commented Oct 24, 2017

added alias field, callback and tags_title inserttag #6

added alias field, callback and tags_title inserttag #6

Conversation

Defcon0 commented Sep 4, 2017

coveralls commented Sep 4, 2017

Defcon0 commented Sep 4, 2017

coveralls commented Sep 4, 2017

coveralls commented Sep 4, 2017

qzminski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 7, 2017

coveralls commented Sep 7, 2017

coveralls commented Sep 7, 2017

coveralls commented Sep 7, 2017

Defcon0 commented Sep 7, 2017

coveralls commented Sep 7, 2017

coveralls commented Sep 7, 2017

coveralls commented Sep 7, 2017

coveralls commented Sep 7, 2017

coveralls commented Sep 8, 2017

coveralls commented Sep 8, 2017

Defcon0 commented Sep 8, 2017

qzminski commented Sep 12, 2017

Defcon0 commented Sep 12, 2017

qzminski commented Oct 24, 2017