New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add tags to posts with many-to-many relation #192
Conversation
*/ | ||
public function addPost(Post $post) | ||
{ | ||
$this->posts[] = $post; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should check whether the post already added, because Doctrine's collection doesn't do this. The same for the tag entity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so it would be as following?
if (false === $this->posts->indexOf($post)) {
$this->posts[] = $post;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should look like this:
public function addTag(Tag $tag)
{
if (!$this->tags->contains($tag)) {
$this->tags->add($tag);
$tag->addPost($this);
}
return $this;
}
public function removeTag(Tag $tag)
{
if ($this->tags->contains($tag)) {
$this->tags->removeElement($tag);
$tag->removePost($this);
}
return $this;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@voronkovich thanks, didn't think of contains(), I will amend the change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice example, thanks 👍
@@ -91,11 +96,114 @@ private function loadPosts(ObjectManager $manager) | |||
} | |||
|
|||
$manager->persist($post); | |||
|
|||
$this->addReference('post-' . $i, $post); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using the reference system while being in the same fixture class is overkill: you don't clear the entity manager, so you could simply keep the same objects. Return an array of posts from this method, and pass them to the other method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good thinking, for while I am wondering why aren't we using multiple fixture classes (fixture class for each entity class) in this example?
merge symfony/symfon-demo master
Fixed all issues mentioned in code review. |
@@ -5,6 +5,13 @@ | |||
{% block main %} | |||
<h1>{{ post.title }}</h1> | |||
|
|||
<p> | |||
{% if 0 < post.tags | length %}Tags: {% endif %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think should put the if
block around the paragraph to avoid rendering an empty one (the same can be done above).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree
@@ -10,6 +10,13 @@ | |||
{{ post.title }} | |||
</a> | |||
</h2> | |||
{% if 0 < post.tags | length %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about updating the repository to load with tags? To avoid lazy loading
And btw, inform what is lazy loading :)
Merge pull request #1 from symfony/master
Conflicts: app/data/blog.sqlite app/data/blog_test.sqlite src/AppBundle/DataFixtures/ORM/LoadFixtures.php
public function removeTag(Tag $tag) | ||
{ | ||
if (!$this->hasTag($tag)) { | ||
$this->tags->removeElement($tag); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it correct condition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@versh23, no. It should be just $this->tags->removeElement($tag);
, without any checkings because the removeElement
function doesn't throw an exception when the element doesn't exist.
merge master
This PR was squashed before being merged into the master branch (closes #447). Discussion ---------- Add tags to Post This implements #44 and it's a complete variant of #192 (On hold since Ago 2016). | Blog Post Index | Post Show | | ---- | --- | | ![tags-index](https://cloud.githubusercontent.com/assets/2028198/22331272/8f0522f8-e398-11e6-8efb-41b8ebb119f4.png) | ![tags-show](https://cloud.githubusercontent.com/assets/2028198/22331381/29aa391a-e399-11e6-9f5a-b09b25a86090.png) | | Post New/Edit | | --- | | ![tagsinput](https://cloud.githubusercontent.com/assets/2028198/22348577/3263128e-e3da-11e6-8b62-25f9720f4275.png) | Also this splits the only one fixture class into three classes (`UserFixtures`, `PostFixtures` and `TagFixtures`): - [x] To prevent mess and complexity. - [x] To avoid post title duplicated (slug issue). - [x] To fix example and explanation of: Sharing objects between fixtures (#192 (comment)). - [x] To add a new example about: Fixture ordering and `DependentFixtureInterface` (dependence between fixtures). Add Translation: - [x] English - [x] 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`](https://select2.github.io/examples.html) or [`chosen`](https://github.com/harvesthq/chosen) js plugins? * [Bootstrap tagsinput](http://bootstrap-tagsinput.github.io/bootstrap-tagsinput/examples/) js plugin? <-- 👍 * [Tokenfield for Bootstrap](http://sliptree.github.io/bootstrap-tokenfield/) js plugin? ### TODO: - [x] Create TagsInputType to handle the post tags collection (Added DataTransformer example) - [x] Add typeaheadjs option to show tags hint. Commits ------- 46a54dd Add tags to Post
This PR was squashed before being merged into the master branch (closes #447). Discussion ---------- Add tags to Post This implements #44 and it's a complete variant of symfony/demo#192 (On hold since Ago 2016). | Blog Post Index | Post Show | | ---- | --- | | ![tags-index](https://cloud.githubusercontent.com/assets/2028198/22331272/8f0522f8-e398-11e6-8efb-41b8ebb119f4.png) | ![tags-show](https://cloud.githubusercontent.com/assets/2028198/22331381/29aa391a-e399-11e6-9f5a-b09b25a86090.png) | | Post New/Edit | | --- | | ![tagsinput](https://cloud.githubusercontent.com/assets/2028198/22348577/3263128e-e3da-11e6-8b62-25f9720f4275.png) | Also this splits the only one fixture class into three classes (`UserFixtures`, `PostFixtures` and `TagFixtures`): - [x] To prevent mess and complexity. - [x] To avoid post title duplicated (slug issue). - [x] To fix example and explanation of: Sharing objects between fixtures (symfony/demo#192 (comment)). - [x] To add a new example about: Fixture ordering and `DependentFixtureInterface` (dependence between fixtures). Add Translation: - [x] English - [x] 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`](https://select2.github.io/examples.html) or [`chosen`](https://github.com/harvesthq/chosen) js plugins? * [Bootstrap tagsinput](http://bootstrap-tagsinput.github.io/bootstrap-tagsinput/examples/) js plugin? <-- 👍 * [Tokenfield for Bootstrap](http://sliptree.github.io/bootstrap-tokenfield/) js plugin? ### TODO: - [x] Create TagsInputType to handle the post tags collection (Added DataTransformer example) - [x] Add typeaheadjs option to show tags hint. Commits ------- 46a54dd Add tags to Post
This PR was squashed before being merged into the master branch (closes #447). Discussion ---------- Add tags to Post This implements #44 and it's a complete variant of symfony/demo#192 (On hold since Ago 2016). | Blog Post Index | Post Show | | ---- | --- | | ![tags-index](https://cloud.githubusercontent.com/assets/2028198/22331272/8f0522f8-e398-11e6-8efb-41b8ebb119f4.png) | ![tags-show](https://cloud.githubusercontent.com/assets/2028198/22331381/29aa391a-e399-11e6-9f5a-b09b25a86090.png) | | Post New/Edit | | --- | | ![tagsinput](https://cloud.githubusercontent.com/assets/2028198/22348577/3263128e-e3da-11e6-8b62-25f9720f4275.png) | Also this splits the only one fixture class into three classes (`UserFixtures`, `PostFixtures` and `TagFixtures`): - [x] To prevent mess and complexity. - [x] To avoid post title duplicated (slug issue). - [x] To fix example and explanation of: Sharing objects between fixtures (symfony/demo#192 (comment)). - [x] To add a new example about: Fixture ordering and `DependentFixtureInterface` (dependence between fixtures). Add Translation: - [x] English - [x] 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`](https://select2.github.io/examples.html) or [`chosen`](https://github.com/harvesthq/chosen) js plugins? * [Bootstrap tagsinput](http://bootstrap-tagsinput.github.io/bootstrap-tagsinput/examples/) js plugin? <-- 👍 * [Tokenfield for Bootstrap](http://sliptree.github.io/bootstrap-tokenfield/) js plugin? ### TODO: - [x] Create TagsInputType to handle the post tags collection (Added DataTransformer example) - [x] Add typeaheadjs option to show tags hint. Commits ------- 46a54dd Add tags to Post
This fixes #44 still need adding tags to admin back-end and I will work on that later.
Posts list view
Post show view