Skip to content
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

fix: nothing personal validator error with bad email value #723

Merged
merged 2 commits into from
May 2, 2023
Merged

fix: nothing personal validator error with bad email value #723

merged 2 commits into from
May 2, 2023

Conversation

GeorgKott
Copy link
Contributor

@GeorgKott GeorgKott commented Apr 30, 2023

superseded: #712

Sorry for my previous pull request. I closed it because after applying the gpg addition, conflicts occurred in the code for earlier commits.

after that

git rebase -i --root --exec 'git commit --amend --no-edit --no-verify -S'

And I thought it would be easier to create a new pull request than to resolve these conflicts.
I also added some tests (integration and unit) that describe the issue I faced.

If you don't apply the changes I made, I will get an error in the tests.

There was 1 error:

1) Tests\Controllers\RegisterTest::testRegisterActionWithBadEmailValue
ErrorException: Undefined offset: 1

\PhpstormProjects\shield\src\Authentication\Passwords\NothingPersonalValidator.php:78
\PhpstormProjects\shield\src\Authentication\Passwords\NothingPersonalValidator.php:27
\PhpstormProjects\shield\src\Authentication\Passwords.php:132
\PhpstormProjects\shield\src\Authentication\Passwords\ValidationRules.php:45
\PhpstormProjects\shield\vendor\codeigniter4\framework\system\Validation\Validation.php:311
\PhpstormProjects\shield\vendor\codeigniter4\framework\system\Validation\Validation.php:170
\PhpstormProjects\shield\vendor\codeigniter4\framework\system\Controller.php:155
\PhpstormProjects\shield\src\Controllers\RegisterController.php:103
\PhpstormProjects\shield\vendor\codeigniter4\framework\system\CodeIgniter.php:934
\PhpstormProjects\shield\vendor\codeigniter4\framework\system\CodeIgniter.php:499
\PhpstormProjects\shield\vendor\codeigniter4\framework\system\CodeIgniter.php:368
\PhpstormProjects\shield\vendor\codeigniter4\framework\system\Test\FeatureTestTrait.php:181
\PhpstormProjects\shield\vendor\codeigniter4\framework\system\Test\FeatureTestTrait.php:226
\PhpstormProjects\shield\tests\Controllers\RegisterTest.php:300

i think that a POST request can contain any data, not necessarily a valid email value. After all, it's not the front-end's responsibility to validate, but the back-end.
Thank you

@datamweb datamweb changed the title fix nothing personal validator error with bad email value fix: nothing personal validator error with bad email value Apr 30, 2023
Copy link
Member

@kenjis kenjis left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you.

@kenjis kenjis merged commit 1225c80 into codeigniter4:develop May 2, 2023
$localPart,
$domain,
] = explode('@', $email);
if (str_contains($email, '@')) {
Copy link
Member

Choose a reason for hiding this comment

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

str_contains() requires PHP8. So using it here is a bug.
But we use polyfill-php80, so the test does not fail on PHP 7.4.

$ composer why symfony/polyfill-php80
friendsofphp/php-cs-fixer v3.16.0 requires symfony/polyfill-php80 (^1.27)

Copy link
Member

Choose a reason for hiding this comment

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

But it is a transitive dependency of a dev dependency, so using it here is still a bug.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it is a bug, but we cannot detect it by testing.
Why don't we require symfony/polyfill-php80?

@kenjis kenjis added the bug Something isn't working label Oct 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants