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

[ParameterValidator] Handle missing parameter type #1057

Merged
merged 2 commits into from
Apr 4, 2019

Conversation

Roliga
Copy link
Contributor

@Roliga Roliga commented Mar 6, 2019

If the 'type' property for a required parameter is not specified it was treated as if it was not required by getQueriedContext since 434c126.

Maybe there's some more readable way to write this but this seems to make it work as intended at least!

If the 'type' property for a required parameter is not specified it was
treated as if it was not required by getQueriedContext.
@logmanoriginal
Copy link
Contributor

You are right, the current condition is wrong if the type is not specified. It actually took me a while to understand how the condition works (which is funny because I wrote it myself... not a good sign).

Your condition is correct though.

Maybe there's some more readable way to write this but this seems to make it work as intended at least!

Indeed. One way this can be simplified is by putting in a guard clause like this (haven't tested it):

// Check if all parameters of the context are satisfied
foreach($set as $id => $properties) {
    if(isset($data[$id]) && !empty($data[$id])) {
        $queriedContexts[$context] = true;
    } elseif (isset($properties['type'] 
    && ($properties['type'] === 'checkbox' || $properties['type'] === 'list'))) {
        continue;
    } elseif(isset($properties['required']) && $properties['required'] === true) {
        $queriedContexts[$context] = false;
        break;
    }
}

The second elseif checks if the type is set and any of the not supported types, in which case the remaining loop is skipped. That should make it easier to understand imo. What do you think?

@Roliga
Copy link
Contributor Author

Roliga commented Mar 26, 2019

Forgot a bit about this for a while, but yes that looks much better to me! I added a commit with the suggested changes.

@teromene teromene merged commit 380fdf2 into RSS-Bridge:master Apr 4, 2019
@teromene
Copy link
Member

teromene commented Apr 4, 2019

Looks good to me ! Thanks for the PR !

infominer33 pushed a commit to web-work-tools/rss-bridge that referenced this pull request Apr 17, 2020
* [ParameterValidator] Handle missing parameter type
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