Skip to content

Conversation

@zebba
Copy link
Contributor

@zebba zebba commented May 23, 2015

Q A
Doc fix? yes
New docs? no
Applies to 2.3+
Fixed tickets #2471

@zebba zebba changed the title Update data_transformers.rst [WIP] Update data_transformers.rst May 23, 2015
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[...] is fine, we will [...]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We avoid the first person perspective in the docs. So we should reword the sentence a bit like the following:

While for most use-cases it is sufficient to use the default entity manager, you will sometimes need to explicitly choose the one to use. To achieve this, you can use a factory::

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you have any reason why you don't use Doctrine\Common\Persistence\ManagerRegistry, but require the specific bridge implementation?

Couple of changes.
@javiereguiluz
Copy link
Member

All suggestions have been implemented, so this PR seems ready to be merged ... or at least to label it as Finished. Thanks.

@zebba zebba changed the title [WIP] Update data_transformers.rst [FINAL] Update data_transformers.rst Jun 11, 2015
@zebba zebba changed the title [FINAL] Update data_transformers.rst Update data_transformers.rst Jun 14, 2015
@weaverryan weaverryan merged commit 939cfc5 into symfony:2.3 Jun 27, 2015
weaverryan added a commit that referenced this pull request Jun 27, 2015
This PR was merged into the 2.3 branch.

Discussion
----------

Update data_transformers.rst

| Q             | A
| ------------- | ---
| Doc fix?      | yes
| New docs?     | no
| Applies to    | 2.3+
| Fixed tickets | #2471

Commits
-------

939cfc5 Update data_transformers.rst
4b09b3f Update data_transformers.rst
7929ed5 Update data_transformers.rst
19324e8 Update data_transformers.rst
@weaverryan
Copy link
Member

GREAT cleanup @zebba! I didn't realize how much this needed it :). So, I ran further in #5456 and overhauled it even more. I'd appreciate your thoughts there!

Thanks!

weaverryan added a commit that referenced this pull request Jul 8, 2015
…averryan)

This PR was merged into the 2.3 branch.

Discussion
----------

Completely re-reading the data transformers chapter

| Q             | A
| ------------- | ---
| Doc fix?      | yes
| New docs?     | no
| Applies to    | 2.3+
| Fixed tickets | #5307

**Note**: `setDefaultOptions()` -> `configureOptions()` in 2.7 after merge

Hi guys!

This follows after #5307, which helped update the article (but also added some new complexity). Data transformers are super cool, but I think we've over-complicated them in the past. Even for me, before re-writing this, I tried to avoid transformers because I thought they were hard :).

The code for this can be found here: https://github.com/weaverryan/docs-data-transformer/commits/finish, where each commit that starts with "[tuts-apply-diff]" is a "step" in the tutorial.

My goals:

1) Show an easy example using the CallbackTransformer in the beginning
2) Remove complexity related to any factories that were there before
3) Moved theoretical section to the bottom

Thanks!

Commits
-------

0762848 Adding an example of how the other format for attaching transformers looks
28d7f89 fixed typo thanks to @OskarStark
41bb907 adding use statement
1514fdd using the aliases entity manager service name
9be07f0 Many tweaks thanks to review
b48feda Completely re-reading the data transformers chapter:
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants