8000 Fix `ConstraintViolation#getPropertyPath()` to always return `string` by Ocramius · Pull Request #40415 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Fix ConstraintViolation#getPropertyPath() to always return string #40415

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 10, 2021
Merged

Fix ConstraintViolation#getPropertyPath() to always return string #40415

merged 1 commit into from
Mar 10, 2021

Conversation

Ocramius
Copy link
Contributor
@Ocramius Ocramius commented Mar 8, 2021
Q A
Branch? 5.2
Bug fix? yes
New feature? no
Deprecations? no
Tickets
License MIT
Doc PR

ConstraintViolation#getPropertyPath()'s inherited signature states that string is
to be returned by it at all times, yet the implementation returns null when no property
path had been provided at instantiation.

This patch obviates it, returning an empty string when the
property path is null.

@Ocramius
Fix ConstraintViolation#getPropertyPath() to always return string
7d1029b 
`ConstraintViolation#getPropertyPath()`'s inherited signature states that `string` is
to be returned by it at all times, yet the implementation returns `null` when no property
path had been provided at instantiation.

This patch obviates it, returning an empty string when the
property path is `null`.
@carsonbot carsonbot added this to the 5.2 milestone Mar 8, 2021
@Ocramius
Copy link
Contributor Author
Ocramius commented Mar 8, 2021

Integration test suite failure seems unrelated

@derrabus
Copy link
Member
derrabus commented Mar 8, 2021

Good catch. We have the same problem with the $messageTemplate property in that class if I'm not mistaken. In both cases, I'd suggest to perform the string cast inside the constructor already. Either way, it looks like a mistake to me that we allow null to be passed to the constructor.

@Ocramius
Copy link
Contributor Author
Ocramius commented Mar 8, 2021

Either way, it looks like a mistake to me that we allow null to be passed to the constructor.

That ship has sailed though, and there's active design around the property path being missing.

@derrabus
Copy link
Member
derrabus commented Mar 8, 2021

That ship has sailed though, and there's active design around the property path being missing.

Yeah, for sure, we cannot just change the type now. But if it's a mistake, we could try to correct it for Symfony 6. The parameter types for $propertyPath and $messageTemplate have been added with #24722 despite the PHPDoc block documenting those parameters as non-nullable.

Our own code (ExecutionContext::addViolation() and ConstraintViolationBuilder::addViolation()) should make sure that we don't pass null to either of them. May I ask how you got yourself a ConstraintViolation instance with a null property path? Maybe I missed something.

@Ocramius
Copy link
Contributor Author
Ocramius commented Mar 8, 2021

Our own code (ExecutionContext::addViolation() and ConstraintViolationBuilder::addViolation()) should make sure that we don't pass null to either of them. May I ask how you got yourself a ConstraintViolation instance with a null property path? Maybe I missed something.

I was copying violations reported from one validator to another, pretty much like ConstraintViolationBuilderInterface#atPath(ConstraintViolationInterface#getPropertyPath())

@Nyholm
Copy link
Member
Nyholm commented Mar 8, 2021

Thank you. Could you do the same fix for the $messageTemplate please? The same bug is for getMessageTemplate() too.

@Ocramius
Copy link
Contributor Author
Ocramius commented Mar 8, 2021

Will send a separate patch
8000

Ocramius added a commit to Ocramius/symfony that referenced this pull request Mar 8, 2021
@Ocramius
Fix ConstraintViolation#getMessageTemplate() to always return string
cb47a35 
`ConstraintViolation#getMessageTemplate()`'s inherited signature states that `string` is
to be returned by it at all times, yet the implementation returns `null` when no message
template had been provided at instantiation.

This patch obviates it, returning an empty string when the
message template is `null`.

Ref: symfony#40415 (comment)
@Ocramius Ocramius mentioned this pull request Mar 8, 2021
Merged
@derrabus
Copy link
Member
derrabus commented Mar 8, 2021

I still think we should perform the string cast inside the constructor though. 🙃

btw: is the 4.4 branch affected as well?

@Nyholm
Copy link
Member
Nyholm commented Mar 8, 2021

Hm, the small benefit to cast it to a string in the constructor is that we can remove the cast in the __toString() method.

@Ocramius
Copy link
Contributor Author
Ocramius commented Mar 8, 2021 •
edited
Loading

I still think we should perform the string cast inside the constructor though. upside_down_face

You do that, you break subclassing types for little benefit.

I can imagine that some subclassing will likely rely on the fact that $this->foo !== null to take internal decisions.

EDIT: keep in mind that the scope of this is a bugfix, not a refactoring. That would probably target 5.x instead.

@Ocramius
Copy link
Contributor Author
Ocramius commented Mar 8, 2021

btw: is the 4.4 branch affected as well?

Possibly, didn't check, since I'd rather leave 4.4 alone. This is the sort of bugfix that I would never ever want to see in an LTS, since there's a subtle change in behavior.

@derrabus
Copy link
Member
derrabus commented Mar 8, 2021

I still think we should perform the string cast inside the constructor though. upside_down_face

You do that, you break subclassing types for little benefit.

$propertyPath is declared private, so a subclass would need access it via the very same getter that you are patching right now.

@Ocramius
Copy link
Contributor Author
Ocramius commented Mar 8, 2021

Ah, true that 👍

@fabpot
Copy link
Member
fabpot commented Mar 10, 2021

Thank you @Ocramius.

@fabpot fabpot merged commit e5f9a89 into symfony:5.2 Mar 10, 2021
fabpot pushed a commit to Ocramius/symfony that referenced this pull request Mar 10, 2021
@Ocramius @fabpot
Fix ConstraintViolation#getMessageTemplate() to always return string
f6e9132 
`ConstraintViolation#getMessageTemplate()`'s inherited signature states that `string` is
to be returned by it at all times, yet the implementation returns `null` when no message
template had been provided at instantiation.

This patch obviates it, returning an empty string when the
message template is `null`.

Ref: symfony#40415 (comment)
Ocramius added a commit to Ocramius/symfony that referenced this pull request Mar 10, 2021
@Ocramius
Fix ConstraintViolation#getMessageTemplate() to always return string
72a464e 
`ConstraintViolation#getMessageTemplate()`'s inherited signature states that `string` is
to be returned by it at all times, yet the implementation returns `null` when no message
template had been provided at instantiation.

This patch obviates it, returning an empty string when the
message template is `null`.

Ref: symfony#40415 (comment)
@fabpot fabpot mentioned this pull request Mar 10, 2021
Merged
fabpot added a commit that referenced this pull request Mar 11, 2021
@fabpot
bug #40416 Fix ConstraintViolation#getMessageTemplate() to always r…
dc8a43b 
…eturn `string` (Ocramius)

This PR was merged into the 5.2 branch.

Discussion
----------

Fix `ConstraintViolation#getMessageTemplate()` to always return `string`

| Q             | A
| ------------- | ---
| Branch?       | 5.2
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       |
| License       | MIT
| Doc PR        |

`ConstraintViolation#getMessageTemplate()`'s inherited signature states that `string` is
to be returned by it at all times, yet the implementation returns `null` when no message
template had been provided at instantiation.

This patch obviates it, returning an empty string when the
message template is `null`.

Ref: #40415 (comment)

Commits
-------

72a464e Fix `ConstraintViolation#getMessageTemplate()` to always return `string`
symfony-splitter pushed a commit to symfony/validator that referenced this pull request Mar 11, 2021
@Ocramius
Fix ConstraintViolation#getMessageTemplate() to always return string
63134a5 
`ConstraintViolation#getMessageTemplate()`'s inherited signature states that `string` is
to be returned by it at all times, yet the implementation returns `null` when no message
template had been provided at instantiation.

This patch obviates it, returning an empty string when the
message template is `null`.

Ref: symfony/symfony#40415 (comment)
symfony-splitter pushed a commit to symfony/validator that referenced this pull request Mar 11, 2021
@fabpot
bug #40416 Fix ConstraintViolation#getMessageTemplate() to always r…
09c5430 
…eturn `string` (Ocramius)

This PR was merged into the 5.2 branch.

Discussion
----------

Fix `ConstraintViolation#getMessageTemplate()` to always return `string`

| Q             | A
| ------------- | ---
| Branch?       | 5.2
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       |
| License       | MIT
| Doc PR        |

`ConstraintViolation#getMessageTemplate()`'s inherited signature states that `string` is
to be returned by it at all times, yet the implementation returns `null` when no message
template had been provided at instantiation.

This patch obviates it, returning an empty string when the
message template is `null`.

Ref: symfony/symfony#40415 (comment)

Commits
-------

72a464e449 Fix `ConstraintViolation#getMessageTemplate()` to always return `string`
@Ocramius Ocramius deleted the fix/ensure-property-path-respects-declared-interface-type branch March 25, 2021 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@Nyholm Nyholm Nyholm approved these changes

@derrabus derrabus derrabus approved these changes

Assignees
No one assigned
Labels
Bug Status: Reviewed
Projects
None yet
Milestone
5.2
Development

Successfully merging this pull request may close these issues.
5 participants
@Ocramius @derrabus @Nyholm @fabpot @carsonbot
0