Skip to content

[Validator] add section for overridden properties #18747

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

Merged
merged 1 commit into from
Aug 28, 2023
Merged

[Validator] add section for overridden properties #18747

merged 1 commit into from
Aug 28, 2023

Conversation

Valmonzo
Copy link

Fixes #14784.

This is my first contribution, hope I did right.
Thanks @HeahDude for your help 🙏🏽

Comment on lines +739 to +745
.. caution::

Note that overriding a property with others constraints in a child class
will not remove the constraints defined in the parent class on that same
property.
Instead, the constraints will be merged for that property.
This is related to Java Language Specification.
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we can get rid of the caution box and write just regular text? What do you think?

The tip below is ok from my side 👍

Copy link
Author

Choose a reason for hiding this comment

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

It was my first intention, but it may deserve to be more highlighted given the potential issues.
Let's wait for other opinions?

@javiereguiluz javiereguiluz merged commit cf164e3 into symfony:5.4 Aug 28, 2023
@javiereguiluz
Copy link
Member

Valmont, thanks and congrats on your first Symfony Docs contribution 🎉

The contents were perfectly fine but, as Oskar also pointed out, I was a bit worried about the proposed structure. The section would have just 2 lines of text and then a long caution and a long tip. We try to avoid that because it breaks the "reading flow" too much.

So, I reword your proposed contents to remove the caution and the tip, but used blod text to warn users about this behavior. See da9e3b7

Thanks!

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.

4 participants