WsdlToPhp / PackageGenerator

Generates a PHP SDK based on a WSDL, simple and powerful, WSDL to PHP
https://providr.io
MIT License
427 stars 73 forks source link

Use schema attribute's attributes to apply validation rule on generated properties #49

Closed mikaelcom closed 8 years ago

mikaelcom commented 8 years ago

Validation rules should be defined using at least these attributes:

The validation rules should be generated based on an option. This option would be enabled by default. If a user does not want the validation, then he'll have to disable the option on the generation.

Does anyone have an opinion on this enhancement? Would it be good/bad/useful/useless/user's pain in the ass?

mikaelcom commented 8 years ago

It would be an extension of the already generated validation rule for enumerations

GroxExMachine commented 8 years ago

Would it be good/bad/useful/useless/user's pain in the ass?

It would be all of this and something, that you can't yet imagine (just wait for an issue). The option to disable validation will handle this cases.

I was a bit surprised when I did not found such validation in code. So, my vote to have them and enabled by default.

mikaelcom commented 8 years ago

No validation rule was implemented yet because I never wanted to constraint the user (even if the Web Service would do it and mostly because the Web Service would do it). The question of who is responsible of the validation was my concern too: the generated classes or the Web Service. My first goal was to provide an easy to use package. Plus, at the time I started this project, I never thought I would reach this point of generated package. Now it's easier to add validation rules than it would have been before.

Do you think validation rules should be gathered in one place, the PackageBase classes or whithin each setter? I would aply this remark/question to the validation on enumerations and array type properties.

thx

GroxExMachine commented 8 years ago

No validation rule was implemented yet because...

there all your reasons matches with what I thought when I found that there no simple validations )

Do you think validation rules should be gathered in one place, the PackageBase classes or whithin each setter?

I just had a discussion about it. There where no one decision. Summary, in this case I prefer simplicity. So, to have validation in setter. Pros:

Cons:

I wanted to know, describe please, how you can implement it in PackageBase classes.

mikaelcom commented 8 years ago

As we speak of validation, shouldn't we think wider by applying validation rule en every setter method? As now each parameter is either typed as a generated Struct class or a valid PHP type (string, int, bool, float), we could imagine validate each value. I'm not necessarily a fan as it means generating more code, plus are we sure it would work anytime? Of course it could be disabled using the option.

I wanted to know, describe please, how you can implement it in PackageBase classes.

I did not think very long about that but it would mean creating a validation class ruler containing each type of validation that could accept a parameter defining the type that has to be validated and the value. It would then throw the exception:

/**
 * Set amexTokenInput value
 * @param \StructType\CardTokenInput $amexTokenInput
 * @return \StructType\Token
 */
public function setAmexTokenInput(\StructType\CardTokenInput $amexTokenInput = null)
{
    \WsdlToPhp\PackageBase\Validator::validate('\StructType\CardTokenInput', $amexTokenInput);
    $this->amexTokenInput = $amexTokenInput;
    return $this;
}

\WsdlToPhp\PackageBase\Validator::validate could accept a namespaced class name, 'string', 'float', 'int', 'bool', 'regexp', 'between', 'max', 'min' (this has to be analyzed, maybe the symfony constraints component can help?) I'm not sure it would be a good idea as it goes against all your Pros for an inline validation.

Then there is two way of doing validation in the generated struct classes:

any additional thought? what would be your preference?

GroxExMachine commented 8 years ago

Thank you for describing. In this case I prefer inline validation in each setter. It has much more Pros than Cons in both ways - in development of this feature and in use of it.

mikaelcom commented 8 years ago

@GroxExMachine If you could try these modifications (on develop branch now), it would be great, thx

GroxExMachine commented 8 years ago

@mikaelcom I plan to test it in this month.

mikaelcom commented 8 years ago

@GroxExMachine any feedback now? :smile: