Add tags to Post #447

yceruto · 2017-01-26T12:41:04Z

This implements #44 and it's a complete variant of #192 (On hold since Ago 2016).

Blog Post Index	Post Show

Post New/Edit

Also this splits the only one fixture class into three classes (UserFixtures, PostFixtures and TagFixtures):

To prevent mess and complexity.
To avoid post title duplicated (slug issue).
To fix example and explanation of: Sharing objects between fixtures (Add tags to posts with many-to-many relation #192 (comment)).
To add a new example about: Fixture ordering and DependentFixtureInterface (dependence between fixtures).

Add Translation:

English
Español

Updated blog.sqlite and blog_test.sqlite DBs.

Handling Tags (Bootstrap-tagsinput):

There is many options to do that:

Form collection to add new tags?
select2 or chosen js plugins?
Bootstrap tagsinput js plugin? <-- 👍
Tokenfield for Bootstrap js plugin?

TODO:

Create TagsInputType to handle the post tags collection (Added DataTransformer example)
Add typeaheadjs option to show tags hint.

stof · 2017-01-26T14:49:00Z

src/AppBundle/DataFixtures/ORM/LoadPostFixtures.php

+
+    private function addRandomTags(Post $post)
+    {
+        $indexes = (array) array_rand(LoadTagFixtures::$names, mt_rand(1, 3));


Can we use 0 as the min number of tags ? Tags are optional, so it makes sense to have untagged posts too

stof · 2017-01-26T14:49:38Z

src/AppBundle/DataFixtures/ORM/LoadPostFixtures.php

 */
-class LoadFixtures extends AbstractFixture implements ContainerAwareInterface
+class LoadPostFixtures extends AbstractFixture implements DependentFixtureInterface, ContainerAwareInterface


IMO, you can remove the Load part of the name, now that there is an entity name

yceruto · 2017-01-26T15:43:49Z

Next form tags approach ( coming soon 😅 )

yceruto · 2017-01-26T20:56:20Z

Ready!

Status: Need Reviews :)

yceruto · 2017-01-27T12:20:26Z

src/AppBundle/Form/DataTransformer/ArrayToStringTransformer.php

+
+        $newNames = array_diff($names, $tags);
+        foreach ($newNames as $name) {
+            $tag = new Tag();


❓ I'm not sure if it's the best place to do that (i.e. Inside a DataTransformer)

yceruto · 2017-01-27T12:24:21Z

src/AppBundle/DataFixtures/ORM/PostFixtures.php

+            'Era brevis ratione est',
+            'Sunt torquises imitari velox mirabilis medicinaes',
+            'Cum mineralis persuadere omnes finises desiderium bi-color',
+            'Bassus fatalis classiss virtualiter transferre de flavum',


Added 15 phrases more to avoid duplicate post title and slug issue.

voronkovich · 2017-01-27T13:08:20Z

src/AppBundle/Form/DataTransformer/ArrayToStringTransformer.php

+ *
+ * @author Yonel Ceruto <[email protected]>
+ */
+class ArrayToStringTransformer implements DataTransformerInterface


@yceruto, I would name this class like TagArrayToStringTransformer, because the current name sounds very common.

Yes, it makes sense, thanks!

voronkovich · 2017-01-27T13:10:17Z

src/AppBundle/Form/DataTransformer/ArrayToStringTransformer.php

+            $names = explode(',', $string);
+        }
+
+        $tags = $this->manager->getRepository(Tag::class)->findBy([


@yceruto, what about to define tags repository as service and inject it instead of the object manager?

I don't see the benefit :/ it could complicate things

voronkovich · 2017-01-27T14:06:12Z

@yceruto, you forgot to call EntityManager::persist for post in BlogController::editAction to save the changes in tags.
https://github.com/symfony/symfony-demo/blob/master/src/AppBundle/Controller/Admin/BlogController.php#L137

yceruto · 2017-01-27T14:42:30Z

@yceruto, you forgot to call EntityManager::persist for post in BlogController::editAction to save the changes in tags.

AFAIK it's not necessary, Doctrine is able to detect the changes on M2M tags property and do it. Likewise for new tags thanks to cascade={"persist"} option.

stof · 2017-01-27T15:38:46Z

src/AppBundle/Form/Type/TagsInputType.php

+    {
+        $builder
+            ->addModelTransformer(new CollectionToArrayTransformer())
+            ->addViewTransformer(new TagArrayToStringTransformer($this->manager))


this should be a model transformer, not a view transformer. The TextType expects its normalized data to be a string. Changing it to be an array of tags could lead to issues (and such issues could appear in a future version of Symfony if some changes are made, even if there is no such issue today).

Changing the type of the normalized data when extending a type is generally not a supported usage of the type.

the code should look like this:

$builder ->addViewTransformer(new TagArrayToStringTransformer($this->manager)) ->addModelTransformer(new CollectionToArrayTransformer()) ;

You are right, nice catch! fixed thanks!

stof · 2017-01-27T15:40:13Z

web/js/main.js

+        // Bootstrap-tagsinput initialization
+        // http://bootstrap-tagsinput.github.io/bootstrap-tagsinput/examples/
+        var $input = $('input[data-toggle="tagsinput"]');
+        var source = new Bloodhound({


this should be done only when $input.length is not 0 (i.e. at least one such input has been found on the page)

yceruto · 2017-01-27T17:16:39Z

src/AppBundle/Form/Type/TagsInputType.php

+    {
+        $builder
+            ->addModelTransformer(new TagArrayToStringTransformer($this->manager))
+            ->addModelTransformer(new CollectionToArrayTransformer())


At sight this looks confusing for newcomers now, looks like the data transformation is (even if it doesn't really happen so):

Tag[] -> String -> Collection -> Array

(i.e. following the order of addition)

What about other alternatives to do the same:

$builder ->addModelTransformer(new DataTransformerChain([ new CollectionToArrayTransformer(), new TagArrayToStringTransformer($this->manager), ])) ;

or better

$builder ->addModelTransformer(new CollectionToArrayTransformer(), true) ->addModelTransformer(new TagArrayToStringTransformer($this->manager), true) ;

Does this make sense or does it simply not matter?

yceruto · 2017-01-28T13:00:49Z

@stof, @voronkovich Thanks for the review. I have addressed your comments.

javiereguiluz · 2017-02-06T08:22:10Z

@yceruto I'm really sorry it took me so long to merge this. I finally did it and I'll propose some tweaks in a separate PR. My only regret is that the solution looks too complex. Don't get me wrong, you did a great work and the feature it's fantastically implemented ... but thinking as a potential Symfony user looking at this demo application, I don't like such a complex solution for such a common feature.

yceruto · 2017-02-06T13:28:59Z

@javiereguiluz I would be happy to improve those parts where the solution is complex, please just tell me which ones and I'll try to propose a better one or comment the code for a better understanding of it.

Thanks, best regards.

javiereguiluz · 2017-02-06T13:55:18Z

@yceruto my comment was more about the complexity imposed by the Form component, such as creating a custom data transformer, enabling two data transformers in the custom form type, calling to the always confusing ->setByReference(false) inside the form type, etc.

This PR was squashed before being merged into the master branch (closes #448). Discussion ---------- Tweaked the new blog post tags feature This continues #447 and introduces some minor changes. --- The only relevant changes for end users is the minor redesign of the tags. For example, in the blog index, before tags stand out too much and they look like buttons: ![index-before](https://cloud.githubusercontent.com/assets/73419/22641958/1681671e-ec59-11e6-8178-e60b0eff9a4f.png) Now they are faded because, after all, they are a secondary information: ![index-after](https://cloud.githubusercontent.com/assets/73419/22641977/22e349d2-ec59-11e6-9974-85565c9a565d.png) In the blog post show, the tags are bigger: ![show-after](https://cloud.githubusercontent.com/assets/73419/22641984/2939dc1a-ec59-11e6-952c-fe1d53dc0b04.png) And in the admin, I haven't modified anything, so they still look like this: ![admin-after](https://cloud.githubusercontent.com/assets/73419/22642002/391c0e6e-ec59-11e6-8e8c-f23f7267bc99.png) Commits ------- f09c564 Tweaked the new blog post tags feature

stof reviewed Jan 26, 2017

View reviewed changes

yceruto commented Jan 27, 2017

View reviewed changes

voronkovich reviewed Jan 27, 2017

View reviewed changes

stof reviewed Jan 27, 2017

View reviewed changes

yceruto commented Jan 27, 2017

View reviewed changes

Add post tags

687893d

Force the framework to call the setter

b3deaba

javiereguiluz added Feature FINISHED labels Feb 6, 2017

javiereguiluz closed this in 963c260 Feb 6, 2017

javiereguiluz mentioned this pull request Feb 6, 2017

Tweaked the new blog post tags feature #448

Closed

yceruto deleted the tags branch February 6, 2017 13:08

javiereguiluz mentioned this pull request Feb 6, 2017

Add tags to posts with many-to-many relation #192

Closed

yceruto mentioned this pull request Feb 7, 2017

Add tags to posts with many-to-many relation #44

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tags to Post #447

Add tags to Post #447

yceruto commented Jan 26, 2017 •

edited

Loading

stof Jan 26, 2017

yceruto Jan 26, 2017

stof Jan 26, 2017

yceruto Jan 26, 2017

yceruto commented Jan 26, 2017

yceruto commented Jan 26, 2017 •

edited

Loading

yceruto Jan 27, 2017 •

edited

Loading

yceruto Jan 27, 2017

voronkovich Jan 27, 2017

yceruto Jan 27, 2017

voronkovich Jan 27, 2017

yceruto Jan 27, 2017 •

edited

Loading

voronkovich commented Jan 27, 2017

yceruto commented Jan 27, 2017 •

edited

Loading

stof Jan 27, 2017

yceruto Jan 27, 2017 •

edited

Loading

stof Jan 27, 2017

yceruto Jan 27, 2017 •

edited

Loading

yceruto commented Jan 28, 2017

javiereguiluz commented Feb 6, 2017

yceruto commented Feb 6, 2017 •

edited

Loading

javiereguiluz commented Feb 6, 2017

Add tags to Post #447

Add tags to Post #447

Conversation

yceruto commented Jan 26, 2017 • edited Loading

Handling Tags (Bootstrap-tagsinput):

TODO:

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

yceruto commented Jan 26, 2017

yceruto commented Jan 26, 2017 • edited Loading

yceruto Jan 27, 2017 • edited Loading

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

yceruto Jan 27, 2017 • edited Loading

Choose a reason for hiding this comment

voronkovich commented Jan 27, 2017

yceruto commented Jan 27, 2017 • edited Loading

Choose a reason for hiding this comment

yceruto Jan 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yceruto Jan 27, 2017 • edited Loading

Choose a reason for hiding this comment

yceruto commented Jan 28, 2017

javiereguiluz commented Feb 6, 2017

yceruto commented Feb 6, 2017 • edited Loading

javiereguiluz commented Feb 6, 2017

yceruto commented Jan 26, 2017 •

edited

Loading

yceruto commented Jan 26, 2017 •

edited

Loading

yceruto Jan 27, 2017 •

edited

Loading

yceruto Jan 27, 2017 •

edited

Loading

yceruto commented Jan 27, 2017 •

edited

Loading

yceruto Jan 27, 2017 •

edited

Loading

yceruto Jan 27, 2017 •

edited

Loading

yceruto commented Feb 6, 2017 •

edited

Loading