-
Notifications
You must be signed in to change notification settings - Fork 231
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 configuration example with PHP
format
#834
Conversation
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.
Sorry for the number of comments 🙈
docs/config.rst
Outdated
->type('xml') | ||
->mapping('MyBundle3') | ||
->type('attribute') | ||
->dir(__DIR__.'/../src/Document') |
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 the directories should be consistent with previous sections, so this should read Documents/
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 constants is one of the advantages of using the PHP configuration, I think it's better to show the full feature as it should be used in the documentation.
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.
@franmomu @malarzm The PR have been updated after your reviews. |
class: Class\Example\Filter\ODM\ExampleFilter | ||
enabled: true | ||
App: | ||
is_bundle: false |
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.
The is_bundle
and alias
configs are not necessary for a default setup. I update the recipe: symfony/recipes-contrib#1582
* MyBundle3: { type: attribute } | ||
* MyBundle4: { type: xml, dir: Resources/config/doctrine/mapping } | ||
* MyBundle5: | ||
* App1: ~ |
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.
This configs need to be updated, as dir
is required when outside of a bundle: https://github.com/symfony/symfony/blob/7bbc9be7ea7d607fa901edb2068c2cea13d24978/src/Symfony/Bridge/Doctrine/DependencyInjection/AbstractDoctrineExtension.php#L68
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 was wondering if we should keep these examples here if we have them in the documentation.
Maybe we can remove them here and update the ones in the documentation adding the dir
option?
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.
Having an example for attribute and xml, and for a bundle would be enough.
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.
Thanks for the changes! 🍻
``Acme\HelloBundle\Document``. | ||
option defaults to the application namespace + ``Document``, for example | ||
for an application called ``App``, the prefix would be | ||
``App\Document``. | ||
|
||
- ``alias`` Doctrine offers a way to alias document namespaces to simpler, |
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 still think we've missed deprecating this one :)
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.
Let's deprecate it for 5.1 and remove it from the doc.
Status: I need to check the examples. I'm not sure everything is correct and optimal. |
Fix #806