Remove placeholder from validarion rules when there is no matching variable

[michalsn]

Description
When we use a placeholder in validation rules and there is no data provided to replace the placeholder, we can end up with something like {variable} as a parameter. This can cause issues for developers like in is_unique validation method.

In this case, when we have rules like: is_unique[user.email,id,{id}] we can produce the query like:

SELECT 1
FROM "user"
WHERE "email" = '[email protected]'
AND "id" != '{id}'
LIMIT 1

In MySQL it would be a mistake we probably could live with, but in PostgreSQL this will produce an error: invalid input syntax for integer since this DB engine checks the field types.

Either way, I would call the current behavior a bug.

This pull request fixes this by removing placeholders that can't be replaced by provided variables.

Related to #2812

Checklist:

• Securely signed commits
• Component(s) with PHPdocs
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[samsonasik]

travis seems failure in 2 places:

1. CodeIgniter\Validation\FormatRulesTest::testRegexMatch
2. CodeIgniter\Validation\ValidationTest::testSplitRulesTrue

[michalsn]

Thanks @samsonasik , it's fixed now.

[jreklund]

With this we are destroying legitimate placed "{data}" in there not allowing us to have custom rules containing regex , as it will destroy them. Or rules matching that literal string in e.g. in_list, sure that placeholder can be changed by the developer.

Personally I'm not a fan of deleting random data in case a match not found in $data. If we want it replaced, the $data array should be populated with an empty string or whatever we want it replaced with.

[michalsn]

From my point of view {data} syntax is reserved for placeholders. Custom rules with regex still are possible but not as parameters since they might be problematic. Sure, there are some trade-offs of this approach, but for me they're reasonable.

Anyway, if this won't be accepted, then we still have #2916 ready to go.

[samsonasik]

I reopened #2916 as alternative

[jreklund]

@michalsn That's just why I have strings with {} syntax saved and validated. And a rule with regex as a placeholder that detects {} in a string. It checks if there are any {} not using the correct format. I have made it universal with said regex instead of hard coded it in. Don't know if other people do custom regex function, but that can cause issues.

And regarding #2916 are that really a problem, as you are missing said Id in the post itself? Sure it dies as it's not valid (throwing an error you can catch), for that database type. But this are also killing strings with {} that can contain real data.

At the moment the function does what it's ment to do, replace placeholder with data provided. These PR will manipulate / alter said data if it does what it's supposed to do.

EDIT: Ok, it's a create call, not having an actual ID in it. So we need to do a check for something like this. Only killing what it's supposed to find. But that will of course only work if they are named the same. 🤔

if ($ignoreValue === "{{$ignoreField}}")
{
	return false;
}

[michalsn]

I'm just saying that I would expect that placeholders should work a little differently - let's say just like variables in a template system. If there is no variable in a template, you won't en-up with a visible placeholder.

But this is just my expectations.

I understand the idea that we should explicitly provide empty values for variables that are not present. But in reality, in day-to-day use, it's very inconvenient. And it forces us to write unnecessary code. I'm trying to change it somehow.

The more I think about it the more I see that my implementation probably isn't the right way. But maybe someone will have some ideas on how to improve this?

[michalsn]

EDIT: Ok, it's a create call, not having an actual ID in it. So we need to do a check for something like this. Only killing what it's supposed to find. But that will of course only work if they are named the same. 🤔

if ($ignoreValue === "{{$ignoreField}}")
{
	return false;
}

Yeah... unfortunately, something like this doesn't guarantee anything. I will keep thinking.

[michalsn]

Ok, I think I will close this PR since it will introduce BC changes anyway. If we are strict about it, then sadly #2916 is also not a way to go.

I guess we should stick with the current implementation but I personally think it will create many confusions in the future.

Developers that use MySQL database probably won't be aware of what query is actually produced when variable for the placeholder is not available. And developers that use PostgreSQL will have no choice but to write some additional code to handle this. Also, these last guys probably will produce a lot of tickets here and threads on forums.

[jreklund]

Hi, I'm not saying you are wrong about pointing out flaws in the system, but it may brake some design concepts. That's why I mentioned it. I don't have any real solutions to the problem itself (PostgreSQL), the ignoreValue === ignoreField only works if we "hard limit" it by giving it a note in the user guide.

But for me #2916 will remove all keys that's named as a translation string, that users can translate themselves. It's not checking is_unique at the moment, as they can't add things. Only update.

Key	Translation
{pages.nextPage}	Next page
{pages.previousPage}	Previous page

[michalsn]

Yes, thank you for pointing all these things our, really. My solution was far from perfect.

I'm just a bit frustrated that we weren't able to find any reasonable solution for this issue.

[jreklund]

I hear ya! Hopefully we can put our heads together and think of something, at the moment I'm totally blank.