Skip to content

Fix factory when validator has no constructor#1605

Merged
alganet merged 1 commit intomainfrom
factory-no-constructor-fix
Jan 15, 2026
Merged

Fix factory when validator has no constructor#1605
alganet merged 1 commit intomainfrom
factory-no-constructor-fix

Conversation

@alganet
Copy link
Member

@alganet alganet commented Jan 15, 2026

This was affecting both v::version and v::attributes, making them impossible to use except for the concrete interface.

This change allows these validators to be built using the chain.

@alganet alganet requested a review from Copilot January 15, 2026 03:48
@codecov
Copy link

codecov bot commented Jan 15, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.47%. Comparing base (abdc73b) to head (9abe993).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #1605   +/-   ##
=========================================
  Coverage     97.46%   97.47%           
- Complexity      991      993    +2     
=========================================
  Files           211      211           
  Lines          2292     2298    +6     
=========================================
+ Hits           2234     2240    +6     
  Misses           58       58           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link

Copilot AI 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 overview

This pull request fixes a bug in the NamespacedValidatorFactory that prevented validators without constructors (like v::version() and v::attributes()) from being instantiated through the builder chain API. The fix checks if a class has a constructor before attempting to pass arguments to it.

Changes:

  • Added logic to handle validators without constructors in NamespacedValidatorFactory
  • Created a test validator NoConstructor to support testing this scenario
  • Added a unit test to verify the factory can create validators that have no constructor

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
library/NamespacedValidatorFactory.php Added check for null constructor before calling newInstanceArgs() to support validators without constructors
tests/library/Validators/NoConstructor.php Created test validator extending Simple with no constructor to support testing
tests/unit/NamespacedRuleFactoryTest.php Added test case and import to verify validators without constructors can be created via factory

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@alganet alganet marked this pull request as ready for review January 15, 2026 03:56
@alganet alganet requested a review from henriquemoody January 15, 2026 03:57
@henriquemoody
Copy link
Member

This is really curious. How come the tests didn't pick this up?

@alganet
Copy link
Member Author

alganet commented Jan 15, 2026

@henriquemoody Now I see the problem better.

I was doing v::version("argument"), and got the "version" is not a valid rule name error. I rushed a fix and it mislead me. The fix works, but perhaps it's not the best choice of what to do (accidentally works).

Perhaps we should throw ""version" has 0 arguments, 1 passed instead.. That would be less misleading than saying it doesn't exist, and still informative to the user.

EDIT: It's kind of a pain to manage mandatory/optional arguments though, and we shouldn't be checking that kind of stuff during runtime (ReflectionArgument and so on are slow).

@henriquemoody
Copy link
Member

@alganet I think maybe we just need better exception handling. Right now there are too many things in the same try block. We could improve that without managing arguments.

An exception saying that a validator is not valid is one thing, but in this case we should say we were not able to create a validator with the given arguments.

Since you are at it already, perhaps you could put the creation of the validator in another try block and handle that exception with a more descriptive message

@alganet
Copy link
Member Author

alganet commented Jan 15, 2026

@henriquemoody makes sense, good idea!

@alganet alganet force-pushed the factory-no-constructor-fix branch from 9d1c9ae to b0875c6 Compare January 15, 2026 05:19
@alganet alganet requested a review from henriquemoody January 15, 2026 05:20
This change introduces a less misleading exception when trying to
construct an instance and failing due to mismatched arguments
coming from ReflectionExceptions.
@alganet alganet force-pushed the factory-no-constructor-fix branch from bd274a0 to 9abe993 Compare January 15, 2026 05:45
@alganet alganet requested a review from henriquemoody January 15, 2026 05:47
@alganet alganet merged commit 4618996 into main Jan 15, 2026
10 checks passed
@alganet alganet deleted the factory-no-constructor-fix branch January 15, 2026 05:51
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.

2 participants