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.

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

Fixed a bug where a newline was treated as a valid value even if it was included at the end. #4094

Merged
merged 2 commits into from
Jan 16, 2021
Merged

Fixed a bug where a newline was treated as a valid value even if it was included at the end. #4094

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
108a73f
fix: treated as false if there is a newline at the end.
ytetsuro Jan 11, 2021
9bdba2e
docs: add regex101.com links.
ytetsuro Jan 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 14 additions & 7 deletions system/Validation/FormatRules.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ public function alpha_space(?string $value = null): bool
return true;
}

return (bool) preg_match('/^[A-Z ]+$/i', $value);
// @see https://regex101.com/r/LhqHPO/1
return (bool) preg_match('/\A[A-Z ]+\z/i', $value);
Copy link
Member

Choose a reason for hiding this comment

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

please add @see https://regex101.com/ link comment so it will be easier to verify in the future if bug/improvement needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@samsonasik

please add @see https://regex101.com/ link comment so it will be easier to verify in the future if bug/improvement needed

Do we write this link in the code where all the regular expressions are written?
I don't see the need to add a link.
I think the attached URL is maintained by an individual, but how long will it be valid?

Copy link
Member

Choose a reason for hiding this comment

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

You can check these:

CodeIgniter4/system/View/Parser.php

Line 576 in 3119fc8

// See https://regex101.com/r/1GIHTa/1

CodeIgniter4/system/View/Parser.php

Line 742 in 3119fc8

// See https://regex101.com/r/BCBBKB/1

CodeIgniter4/utils/Rector/UnderscoreToCamelCaseVariableNameRector.php

Line 28 in 3119fc8

* @see https://regex101.com/r/OtFn8I/1

Yes, not everwhere yet, but I think it will be better to add it whenever possible for better future.

I've no information about how long it will be valid, but it seems it long enough ,for example, this is from 5 years ago https://regex101.com/library/wL5mP7 .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will this link really be easy to maintain?
Every time a regular expression is edited, does the URL also need to be maintained, and will all the maintainers review and manage it?

The system also has the ability to delete the URLs that are created.
After the merge, if the link is deleted, the information will be lost.
I think test code is more constructive than a link that may or may not work.

What do other MEMBERS think about this?
I don't want to add this URL as my commit, but I will reluctantly add it if many members agree with this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I vote for adding the link.
As it is positioned directly above the code I would think it is implicit to maintain it (or at least see what it does) when changing the regex itself.
Also, while of course the link might point to a dead end some future day, I consider it to be better to provide information that might get lost instead of not providing info at all in the first place.

Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to have a link. It will make our life easier in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for giving me your input.
I have added your comment.

}

/**
Expand All @@ -56,7 +57,8 @@ public function alpha_space(?string $value = null): bool
*/
public function alpha_dash(?string $str = null): bool
{
return (bool) preg_match('/^[a-z0-9_-]+$/i', $str);
// @see https://regex101.com/r/XfVY3d/1
return (bool) preg_match('/\A[a-z0-9_-]+\z/i', $str);
}

/**
Expand All @@ -72,7 +74,8 @@ public function alpha_dash(?string $str = null): bool
*/
public function alpha_numeric_punct($str)
{
return (bool) preg_match('/^[A-Z0-9 ~!#$%\&\*\-_+=|:.]+$/i', $str);
// @see https://regex101.com/r/6N8dDY/1
return (bool) preg_match('/\A[A-Z0-9 ~!#$%\&\*\-_+=|:.]+\z/i', $str);
}

/**
Expand All @@ -96,7 +99,8 @@ public function alpha_numeric(?string $str = null): bool
*/
public function alpha_numeric_space(?string $str = null): bool
{
return (bool) preg_match('/^[A-Z0-9 ]+$/i', $str);
// @see https://regex101.com/r/0AZDME/1
return (bool) preg_match('/\A[A-Z0-9 ]+\z/i', $str);
}

/**
Expand All @@ -123,7 +127,8 @@ public function string($str = null): bool
*/
public function decimal(?string $str = null): bool
{
return (bool) preg_match('/^[-+]?[0-9]{0,}\.?[0-9]+$/', $str);
// @see https://regex101.com/r/HULifl/1/
return (bool) preg_match('/\A[-+]?[0-9]{0,}\.?[0-9]+\z/', $str);
}

/**
Expand All @@ -147,7 +152,7 @@ public function hex(?string $str = null): bool
*/
public function integer(?string $str = null): bool
{
return (bool) preg_match('/^[\-+]?[0-9]+$/', $str);
return (bool) preg_match('/\A[\-+]?[0-9]+\z/', $str);
}

/**
Expand Down Expand Up @@ -181,7 +186,8 @@ public function is_natural_no_zero(?string $str = null): bool
*/
public function numeric(?string $str = null): bool
{
return (bool) preg_match('/^[\-+]?[0-9]*\.?[0-9]+$/', $str);
// @see https://regex101.com/r/bb9wtr/1
return (bool) preg_match('/\A[\-+]?[0-9]*\.?[0-9]+\z/', $str);
}

/**
Expand Down Expand Up @@ -253,6 +259,7 @@ public function valid_json(string $str = null): bool
*/
public function valid_email(string $str = null): bool
{
// @see https://regex101.com/r/wlJG1t/1/
if (function_exists('idn_to_ascii') && defined('INTL_IDNA_VARIANT_UTS46') && preg_match('#\A([^@]+)@(.+)\z#', $str, $matches))
{
$str = $matches[1] . '@' . idn_to_ascii($matches[2], 0, INTL_IDNA_VARIANT_UTS46);
Expand Down
28 changes: 28 additions & 0 deletions tests/system/Validation/FormatRulesTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,10 @@ public function alphaSpaceProvider()
FormatRulesTest::ALPHABET . ' ',
true,
],
[
FormatRulesTest::ALPHABET . "\n",
false,
],
[
FormatRulesTest::ALPHABET . '1',
false,
Expand Down Expand Up @@ -506,6 +510,10 @@ public function alphaNumericPunctProvider()
FormatRulesTest::ALPHANUMERIC . '`',
false,
],
[
FormatRulesTest::ALPHANUMERIC . "\n",
false,
],
[
FormatRulesTest::ALPHANUMERIC . '@',
false,
Expand Down Expand Up @@ -599,6 +607,10 @@ public function alphaNumericSpaceProvider()
' ' . FormatRulesTest::ALPHANUMERIC . '-',
false,
],
[
' ' . FormatRulesTest::ALPHANUMERIC . "\n",
false,
],
[
null,
false,
Expand Down Expand Up @@ -636,6 +648,10 @@ public function alphaDashProvider()
FormatRulesTest::ALPHANUMERIC . '-\ ',
false,
],
[
FormatRulesTest::ALPHANUMERIC . "-\n",
false,
],
[
null,
false,
Expand Down Expand Up @@ -722,6 +738,10 @@ public function numericProvider()
'+42',
true,
],
[
"+42\n",
false,
],
[
'123a',
false,
Expand Down Expand Up @@ -771,6 +791,10 @@ public function integerProvider()
'-1',
true,
],
[
"+42\n",
false,
],
[
'123a',
false,
Expand Down Expand Up @@ -824,6 +848,10 @@ public function decimalProvider()
'0',
true,
],
[
"0\n",
false,
],
[
'1.0a',
false,
Expand Down