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

feature(web_services): allows calling WS functions with associative array #9406

Closed
wants to merge 2 commits into from

Conversation

mrclay
Copy link
Member

@mrclay mrclay commented Feb 22, 2016

(this was replaced by #9418)

Plugins can expose their methods in a way that invokes callback functions with a single argument that contains an associative array of parameters, rather than each parameter being its own argument.

Deprecates verify_parameters() and serialise_parameters() that are no longer in use.

Updates unit tests to include new features, as well as fixes some code issues to match test expectations

Allows all callables as method callback.

(this rebases #8615 and makes it mostly BC with 2.0)

@beck24
Copy link
Member

beck24 commented Feb 22, 2016

Awesome, this is a feature I'll be using right away.
LGTM

@hypeJunction
Copy link
Contributor

I don't see what changed from the original PR :)

// $this->assertIsA($e, 'APIException');
// $this->assertIdentical($e->getMessage(), sprintf(elgg_echo('APIException:InvalidParameter'), 'param1', 'test'));
// }
// }
Copy link
Member

Choose a reason for hiding this comment

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

Why has this been commented?

@mrclay
Copy link
Member Author

mrclay commented Feb 22, 2016

I'm going to move some of these changes to a 2.0 PR.

…rray

Plugins can expose their methods in a way that invokes callback functions
with a single argument that contains an associative array of parameters,
rather than each parameter being its own argument.

Deprecates verify_parameters() and serialise_parameters() that are no longer
in use.

Updates unit tests to include new features, as well as fixes some code issues
to match test expectations.

Removes eval() magic and serialization from web services to allow for a wider
range of callables as method callback.

Improves handling and validation of parameters.
@mrclay
Copy link
Member Author

mrclay commented Feb 23, 2016

Please see #9411. If we decide it can be fixed in 2.x we can merge this as is.

Casting follows 2.0 behavior and adds more thorough tests.
Throw InvalidParameterException if non-array passed in.
Empty arrays are valid as array input.
An explicit "default" key must be given as a default to be used.
A falsy default value should not be considered missing.
Re-add testVerifyParametersTypeNotSet.
@mrclay
Copy link
Member Author

mrclay commented Feb 23, 2016

For more BC with 2.0 I've added b07e645. So basically this PR adds assoc calling, fixes #9411, and allows all callables. Reforming how string values are cast would have a big BC impact. If that's something urgent the desired behavior should be documented in a separate issue.

@mrclay
Copy link
Member Author

mrclay commented Feb 23, 2016

The feature/fix this PR provides churns the code a bunch for BC concerns and I'd rather put a cleaner PR together to avoid accidentally modifying behavior in 2.x.

@mrclay mrclay closed this Feb 23, 2016
@hypeJunction
Copy link
Contributor

We are back to the question if it's worth the effort. You might as well invest that time into a complete rewrite.

@mrclay
Copy link
Member Author

mrclay commented Feb 23, 2016

I can give it another few min, worst case I'll just reopen this.

@beck24
Copy link
Member

beck24 commented Feb 23, 2016

wasn't one of the arguments for separating the core plugins that we don't have to be too concerned about BC within plugins? We can release a new major semver release of webservices, people who really require the old behavior can just use the old one. Maybe this should/could be a trial run with that.

@mrclay mrclay deleted the ws_assoc branch February 24, 2016 00:43
@mrclay
Copy link
Member Author

mrclay commented Feb 24, 2016

@beck24 yeah we can do that. #9419

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants