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

Implement and Test NFR #10866 #10868

Closed
wants to merge 3 commits into from
Closed

Implement and Test NFR #10866 #10868

wants to merge 3 commits into from

Conversation

lajosbencz
Copy link

NFR #10866

@andresgutierrez
Copy link
Contributor

This looks too arbitrary, not sure if everyone wants to ignore empty strings #440

return filter->sanitize(paramValue, filters);
let paramValue = filter->sanitize(paramValue, filters);
if strlen(paramValue) < 1 && defaultValue !== null {
return defaultValue;
Copy link
Contributor

Choose a reason for hiding this comment

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

Mb

return filter->sanitize(defaultValue, filters);

?

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense!

@lajosbencz
Copy link
Author

Imho, I can't really think of a case when a 0 length dispatcher param is useful after sanitation... not that others can't, but one will most likely handle it somehow, or just skip a prog branch. I think it's counter-intuitive if one still gets an empty string if a defaultValue is provided.
If only sanitation is the objective, defaultValue can be left empty.
I'm not keen on this implementation, because I don't need it in any project atm... nor is it something cardinal. It just makes sense for me this way :)
Also, dispatcher is a bit different than models or forms.

Sticking with the Mvc implementation, consider these (albeit limited and not allowing for proper routing patterns) examples for int validation:

  • user/view/123 will display user from db
  • user/view/awd will get any empty string as id, there will be no returned rows. Page will either be left empty or routed to 404 with if/else
    Although these cases will not benefit from a defaultValue, but I don't think the developer would provide one in the first place.
  • category/view/123 proper page is displayed
  • category/view/awd There would be no rows. Now defaultValue can be useful, instead of displaying a not found and handling it with if/else, developers could provide a defaultValue for a common/promo'd category.

@lajosbencz
Copy link
Author

On second thought, adding a hasParam method would also allow for more flexibility without any impairment

@sergeyklay sergeyklay closed this Jan 7, 2016
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.

3 participants