Skip to content

[12.x] Refactor duplicated logic in ReplacesAttributes #56789

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
Aug 27, 2025
Merged

[12.x] Refactor duplicated logic in ReplacesAttributes #56789

merged 1 commit into from
Aug 27, 2025
+5 −24

Conversation

AhmedAlaa4611
Copy link
Contributor

Description

This PR refactors several validation message replacer methods that shared the same logic:

  • replaceAcceptedIf()
  • replaceDeclinedIf()
  • replaceMissingIf()
  • replacePresentIf()
  • replaceRequiredIf()
  • replaceProhibitedIf()

Instead of duplicating the same code, these methods now delegate to the existing replaceAcceptedIf() implementation.

This follows the same approach already used in replacePresentWithAll(), where the logic is centralized in one method and other similar methods simply call it.

framework/src/Illuminate/Validation/Concerns/ReplacesAttributes.php

Lines 449 to 452 in aa671fd

protected function replacePresentWithAll($message, $attribute, $rule, $parameters)
{
return $this->replacePresentWith($message, $attribute, $rule, $parameters);
}

This change reduces code duplication and makes the code easier to maintain.

@AhmedAlaa4611 AhmedAlaa4611 mentioned this pull request Aug 27, 2025
Merged
@taylorotwell taylorotwell merged commit 95eeea4 into laravel:12.x Aug 27, 2025
62 checks passed
@AhmedAlaa4611 AhmedAlaa4611 mentioned this pull request Aug 27, 2025
Merged
@browner12
Copy link
Contributor

I'm all for deduplication, but is there a true connection between the different methods, or is it happenstance they share the same code? I worry we're making an arbitrary connection between them.

Also, how did you choose the "master" method? Would it make more sense to extract some generic method?

@AhmedAlaa4611
Copy link
Contributor Author

Hello @browner12, if you check the 11.x branch, you’ll see that the changes I made aren’t already there. If you search for return $this->, you’ll find that there are already several methods following the same approach, such as:

  • replaceDifferent()
  • replaceDigitsBetween()
  • replaceMissingWithAll()
  • replaceNotIn()
  • replacePresentWithAll()

And you will find additional methods as well. I just stuck with the same approach.

@AhmedAlaa4611
Copy link
Contributor Author

And here is the original PR (for reference) that introduced the replacePresentWithAll() method: #48908

@browner12
Copy link
Contributor

thanks for the reference.

fair, I do love consistency.

not a hill I'll die one, but seems a little arbitrary which method happens to be the source of truth.

probably okay for now. might bite us in the future for a refactor, but we can address then if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@AhmedAlaa4611 @browner12 @taylorotwell