Skip to content

Update validator tests. #18368

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
Apr 22, 2025
Merged

Update validator tests. #18368

merged 1 commit into from
Apr 22, 2025

Conversation

ADmad
Copy link
Member

@ADmad ADmad commented Apr 22, 2025

Ensure validation process works as expected after adding multiple rules using Validator::add(). Refs #18367

Ensure validation process works as expected after adding multiple
rules using Validator::add(). Refs #18367
@ADmad ADmad added this to the 5.2.3 milestone Apr 22, 2025
@dereuromark
Copy link
Member

My guess is that the reflection added is off once we have extended Table classes in between.
I will try to find a way to clarify/reproduce.

@dereuromark
Copy link
Member

I found the issue:
In RulesProvider you slice it off again, commenting it out makes it also work:

    if ($argument->getName() !== 'context') {
        //$arguments = array_slice($arguments, 0, -1);
    }

I guess that happens because the new code by reflection checks already only adds the context conditionally:

    if ($lastParam && $lastParam->getName() === 'context') {
        $args['context'] = $context;
    }

    $result = $callable(...$args);

So in certain cases it gets not added, but then "stripped" even though it was never there.

@ADmad
Copy link
Member Author

ADmad commented Apr 22, 2025

In RulesProvider you slice it off again, commenting it out makes it also work:

That's done in reference to the issue you submitted #17631.

@dereuromark
Copy link
Member

dereuromark commented Apr 22, 2025

Yeah, that whole context thing being randomly at positions is bonkers in such an API.
Quite problematic.

In terms of BC, I wonder how we can fix things up here.

@dereuromark
Copy link
Member

dereuromark commented Apr 22, 2025

Ah, and my application is using

namespace App\Validation;

use Cake\Validation\Validation as CoreValidation;
use PhpCollective\DecimalObject\Decimal;

class Validation extends CoreValidation {

	/**
	 * @inheritDoc
	 */
	public static function decimal(mixed $check, int|bool|null $places = null, ?string $regex = null): bool {
		if ($check instanceof Decimal) {
			$check = (string)$check;
		}

		return parent::decimal($check, $places, $regex);
	}

}

So I guess this could be the issue how to reproduce it.

@dereuromark
Copy link
Member

OK, now that I remember the "context" of the ticket/issue:
Remember how we found a workaround?
dereuromark/cakephp-decimal@246a6c8

Well, turns out with the updates around reflection we must remove that workaround.

So removing

// in your bootstrap
\Cake\Validation\Validator::addDefaultProvider('default', new \Cake\Validation\RulesProvider(\App\Validation\Validation::class));

makes it work

Do we need to fix then something? As this could have other people run into the same trap if they modified this.
Or do we just ignore it as "works-for-me" ?

@ADmad
Copy link
Member Author

ADmad commented Apr 22, 2025

I have already poked at the validator classes multiple times and each time fixing one issue seems to create another one. So I not looking forward to poking it again.

@dereuromark
Copy link
Member

We can merge this and maybe @markstory or someone wants to comment on the open ticket regarding the strategy moving forward. Then we can close this topic now that we know how to solve it.

@dereuromark dereuromark merged commit 23bf239 into 5.x Apr 22, 2025
13 checks passed
@dereuromark dereuromark deleted the issue-18367 branch April 22, 2025 20:37
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