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

[fw-bundle] Add support for validator auto-mapping #559

Merged
3 commits merged into from May 12, 2019

Conversation

dunglas
Copy link
Member

@dunglas dunglas commented Mar 31, 2019

Q A
License MIT

Enables symfony/symfony#27735 by default.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request passes validation.

# Enables validator auto-mapping support.
# For instance, basic validation constraints will be inferred from Doctrine's metadata.
auto_mapping:
App\Entity\: []
Copy link
Member

Choose a reason for hiding this comment

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

I keep thinking that this might make more sense as an annotation o the class not here. Currently, I look at my class to find all my constraints. Now I will possibly have these “silent” constraints I can’t track down. Could we activate the auto-constraints... with a constraint? We could generate entities with this by default with make:entity.

I know, wrong place to discuss - I just didn’t make it in time to the original Pr.

Choose a reason for hiding this comment

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

This is a config which applies to all entities in given namespace, not specific class. How would you do that with annotation? Where would you put such annotation?

Copy link
Member Author

Choose a reason for hiding this comment

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

This config supports setting full namespaces, as here, but also a list of specific classes using the glob syntax.
There is no annotations because it’s a bundle config. Only XML and YAML are supported. It’s similar to the service discovery config.

Copy link
Member Author

Choose a reason for hiding this comment

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

And in almost all cases, it’s safer to always enable autovalidation. If it triggers a false positive, it’s most likely because the user is doing something very wrong that should be fixed.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request does not pass validation.

@dunglas
Copy link
Member Author

dunglas commented May 9, 2019

Recipe updated to use symlinks. Is it ok to use symlinks for whole directories too? (It's why the check is red right now).

I tested it with 4.3 beta and DoctrineBundle master, and it works as expected.

@nicolas-grekas
Copy link
Member

Is it ok to use symlinks for whole directories too? (It's why the check is red right now).

Yes conceptually, no technically apparently :) @fabpot can we adapt the check?

@fabpot
Copy link
Member

fabpot commented May 10, 2019

@symfony-flex-server review

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request does not pass validation.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request does not pass validation.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request does not pass validation.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request does not pass validation.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request does not pass validation.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request does not pass validation.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request does not pass validation.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request does not pass validation.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request does not pass validation.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request passes validation.

@fabpot
Copy link
Member

fabpot commented May 10, 2019

@symfony-flex-server review

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request passes validation.

@fabpot
Copy link
Member

fabpot commented May 10, 2019

@dunglas @nicolas-grekas Flex server fixed, recipe is green, BUT you should test it to be sure that it works correctly.

@dunglas
Copy link
Member Author

dunglas commented May 11, 2019

On my machine (Mac OS 10.14), symlinked files are not created during composer create-project, even for the current stable version.

Reproducer:

$ composer create-project symfony/skeleton testsf42
$ ls -a testsf42/config/packages/
dev            framework.yaml routing.yaml   test

cache.yaml hasn't been generated.

@nicolas-grekas
Copy link
Member

I confirm the issue: a project created a few days ago contains config/packages/cache.yaml while a new one created now doesn't.

@fabpot
Copy link
Member

fabpot commented May 11, 2019

Problem with symlinked files is fixed now.

@fabpot
Copy link
Member

fabpot commented May 11, 2019

And this PR seems to work fine. @dunglas Can you confirm?

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request passes validation.

@dunglas
Copy link
Member Author

dunglas commented May 11, 2019

Good idea. It's where this config belongs. Updated.

@dunglas
Copy link
Member Author

dunglas commented May 11, 2019

And I confirm this PR works as expected!

@ghost ghost merged commit 32dd187 into symfony:master May 12, 2019
ghost pushed a commit that referenced this pull request May 12, 2019
This pull request was closed.
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.

None yet

8 participants