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

More concrete explanation of validation groups #4454

Closed
wants to merge 1 commit into from
Closed

More concrete explanation of validation groups #4454

wants to merge 1 commit into from

Conversation

peterrehm
Copy link
Contributor

Q A
Doc fix? yes
New docs? no
Applies to all
Fixed tickets -

Improved the explanation of default validation groups in reference with my discussion with @webmozart in symfony/symfony#�11880.

$groups = array_merge($groups, array('company'));
}

return $groups;
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be easier to replace lines 569 to 577 by something like this?

if (Entity\Client::TYPE_PERSON == $data->getType()) {
    return array('Default', 'person');
}

return array('Default', 'company');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could be changed that way if wanted, I prefer it this way to show that the Default is the base and you are adding based on your domain requirements.

Copy link
Contributor

Choose a reason for hiding this comment

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

xabbuh 👍

Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit in favor of @xabbuh's proposal. Shorter code (without making it less readable) is easier to understand most of the time

@peterrehm
Copy link
Contributor Author

@xabbuh Should be finished from my side.

.. caution::::

You must be aware that there is a substantial difference between the ``Default`` and the
``User`` group. Both do not have any different effect on the ``Uer`` entity. However, if
Copy link
Contributor

Choose a reason for hiding this comment

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

Uer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, is updated.

@peterrehm
Copy link
Contributor Author

Updated as per @weaverryan's comments. I would be in favor of getting this merged and the extract or add a cookbook entry.


.. caution::

You must be aware that there is a substantial difference between the ``Default``group and the
Copy link
Member

Choose a reason for hiding this comment

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

missing space after Default

@peterrehm
Copy link
Contributor Author

Updated. I have combined Ryans wording with my information about inheritance.

weaverryan added a commit that referenced this pull request Jan 3, 2015
This PR was submitted for the master branch but it was merged into the 2.3 branch instead (closes #4454).

Discussion
----------

More concrete explanation of validation groups

| Q             | A
| ------------- | ---
| Doc fix?      | yes
| New docs?     | no
| Applies to    | all
| Fixed tickets | -

Improved the explanation of default validation groups in reference with my discussion with @webmozart in symfony/symfony#�11880.

Commits
-------

81728b6 More concrete explanation of validation groups
@weaverryan
Copy link
Member

Yay! I've merged this in - thanks Peter for "exploring" this with us. I did add one more commit, which does need review for accuracy: sha: 09f9d3d. This re-words the last paragraph, but I'm actually not sure if it's accurate. Please let me know if it's not.

Thanks!

@weaverryan weaverryan closed this Jan 3, 2015
weaverryan added a commit that referenced this pull request Jan 3, 2015
* 2.3: (33 commits)
  Fixing bad reference
  [#4743] parts -> part - subjective, but this sounds slightly better to me
  Update web_server_configuration.rst
  Update web_server_configuration.rst
  Update web_server_configuration.rst
  Add exception to console exception log
  [#4454] Re-wording section, but it may not actually be accurate
  More concrete explanation of validation groups
  Adding missing index/map documents
  [#4611] A few more tweaks and fixes
  Basically copying a section about upgrading other libraries down into the minor version section
  [#4611] Removing a few entries I meant to remove before
  [#4611] Making many tweaks thanks to guys like Javier, Wouter, Christian (xabbuh) and Stof
  Adding a guide about upgrading
  Fix typo and remove redundant sentence
  [Reference] Add default_locale config description
  Clarify tip for creating a new AppBundle
  Update forms.rst
  Rewroded some contents and fixed some lists of elements
  MAde some tweaks suggested by Wouter
  ...

Conflicts:
	book/http_cache.rst
weaverryan added a commit that referenced this pull request Jan 3, 2015
* 2.5: (35 commits)
  Fixing bad reference
  [#4743] parts -> part - subjective, but this sounds slightly better to me
  Update web_server_configuration.rst
  Update web_server_configuration.rst
  Update web_server_configuration.rst
  Add exception to console exception log
  [#4454] Re-wording section, but it may not actually be accurate
  More concrete explanation of validation groups
  Adding missing index/map documents
  [#4611] A few more tweaks and fixes
  Basically copying a section about upgrading other libraries down into the minor version section
  [#4611] Removing a few entries I meant to remove before
  [#4611] Making many tweaks thanks to guys like Javier, Wouter, Christian (xabbuh) and Stof
  Adding a guide about upgrading
  Fix typo and remove redundant sentence
  document the `2.5` validation options
  [Reference] Add default_locale config description
  Clarify tip for creating a new AppBundle
  Update forms.rst
  Rewroded some contents and fixed some lists of elements
  ...

Conflicts:
	book/security.rst
weaverryan added a commit that referenced this pull request Jan 3, 2015
* 2.7: (36 commits)
  Fixing bad reference
  [#4743] parts -> part - subjective, but this sounds slightly better to me
  Update web_server_configuration.rst
  Update web_server_configuration.rst
  Update web_server_configuration.rst
  Add exception to console exception log
  [#4454] Re-wording section, but it may not actually be accurate
  More concrete explanation of validation groups
  Adding missing index/map documents
  [#4611] A few more tweaks and fixes
  Basically copying a section about upgrading other libraries down into the minor version section
  [#4611] Removing a few entries I meant to remove before
  [#4611] Making many tweaks thanks to guys like Javier, Wouter, Christian (xabbuh) and Stof
  Adding a guide about upgrading
  Fix typo and remove redundant sentence
  Formatting fix
  document the `2.5` validation options
  [Reference] Add default_locale config description
  Clarify tip for creating a new AppBundle
  Update forms.rst
  ...
@peterrehm peterrehm deleted the validation-groups branch January 4, 2015 11:03
weaverryan added a commit that referenced this pull request Jan 5, 2015
This PR was submitted for the 2.7 branch but it was merged into the 2.3 branch instead (closes #4775).

Discussion
----------

Corrected validation information on inheritance

| Q             | A
| ------------- | ---
| Doc fix?      | yes
| New docs?     | no
| Applies to    | all
| Fixed tickets | -
| Related | #4454

Minor fix as per Discussion 09f9d3d#commitcomment-9150852

Commits
-------

230c4fc Corrected validation information on inheritance
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.

5 participants