Skip to content
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

Open
wants to merge 5 commits into
base: 5.0.x
Choose a base branch
from

Conversation

GromNaN
Copy link
Member

@GromNaN GromNaN commented Jan 4, 2024

Fix #806

@GromNaN GromNaN added this to the 5.0.0 milestone Jan 4, 2024
Copy link
Member

@malarzm malarzm left a 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')
Copy link
Member

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/

Copy link
Member Author

@GromNaN GromNaN Jan 4, 2024

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.

docs/config.rst Outdated Show resolved Hide resolved
docs/config.rst Outdated Show resolved Hide resolved
docs/config.rst Outdated Show resolved Hide resolved
docs/config.rst Outdated Show resolved Hide resolved
docs/config.rst Outdated Show resolved Hide resolved
docs/config.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@franmomu franmomu left a comment

Choose a reason for hiding this comment

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

I remembered that I starting changing references to bundle to fix #536, we should remove them here as well (I've just created #835)

@GromNaN
Copy link
Member Author

GromNaN commented Jan 8, 2024

@franmomu @malarzm The PR have been updated after your reviews.
I updated the first example to take the default configuration provided in the Symfony recipe.. The is_bundle part is very important since Bundle structure is no longer the default.

class: Class\Example\Filter\ODM\ExampleFilter
enabled: true
App:
is_bundle: false
Copy link
Member Author

@GromNaN GromNaN Jan 8, 2024

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: ~
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@franmomu franmomu Jan 10, 2024

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?

Copy link
Member Author

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.

Copy link
Member

@malarzm malarzm left a 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,
Copy link
Member

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 :)

Copy link
Member Author

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.

@GromNaN GromNaN modified the milestones: 5.0.0, 5.0.1, 5.0.2 Jan 15, 2024
@GromNaN
Copy link
Member Author

GromNaN commented Mar 11, 2024

Status: I need to check the examples. I'm not sure everything is correct and optimal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add configuration example with PHP format
4 participants