[PHP] Fix array handling #2726
[PHP] Fix array handling #2726
Conversation
Test if we implement the ArrayAccess interface correct on out model objects.
`parent` in a model is set not only when the model inherits from another model but also when a parent container exists. So We will now use `parentSchema` to check whether a parent class exists. If si we still use `parent` to output the class name because that has been converted to a proper model name and `parentSchema` hasn't.
The models didn't implement a generally working ArrayAccess interface. This would fail on list container types (array). This commit refactors some internals of the model object. The model properties are no longer stored as separate properties on the PHP object but as entries in a `$container` property. This is needed to make the model work also for list containers. Besides it avoids potential problems where the model would specify property names that could collide with names used by the Swagger model implementation itself (i.e. `$attributeMap`).
I didn't have an answer either as I'm not the one adding it. I think it's a good time to rethink again whether we need to implement ArrayAccess for regular model and I don't mind if it's break change (non-backward compatible) if we've a good reason not to implement it. |
|
Re: ArrayAccess I found that stripe-php and google-api-client-php also implements ArrayAccess in their base model: My take is to keep it for now unless it's causing issue. |
|
@arnested I'll review this by coming Wed. |
|
@arnested Changes merged into master. Thanks for the fix. |
|
@arnested as part of the merge, I resolved the conflicts due to better enum support PR. If you've time, I wonder if you can take a look at the latest master to ensure it looks good from your perspective. |
|
The merge was fine. I discovered a minor flaw though when using derived classes (default values from the superclass would not work and a lot of PHP notices would occur when using properties from the superclass as well). A fix and a test is present in #2793. |
This is an attempt at solving #2625.
The problem is we don't handle models defined as arrays correct in (at least) two different ways:
is turned into:
It will only work with associative arrays. Which is fine for all other models but arrays.
For now this pull request is just for gathering feedback on different ways of solving this -- because I'm not sure the current approach is the best.
Feedback is appreciated.
Invalid PHP
The problem comes from the fact that
DefaultCodegen.javaadds theparentvariable to for arrays (line 2194 as well as for objects inheriting from a parent (line 969).The solution I've tried here (bbe12c1) is using the
parentSchemavariable for testing whether a parent object exists asparentSchemais only added when inheriting from another object and not when we have an array.I'm not particular satisfied with this approach. Mostly because I'm not sure of the semantic meaning of
parentSchema. I just used it because the code tells me it will be set in the right cases. And then it looks odd to do testing onparentSchemaand the outputingparent.Another approach would be to add an
isListContainerproperty to CodegenModel and use that combined with the parent variable to decide if this is an array or object inheritance.ArrayAccess interface implementation
To fix the ArrayAccess interface implementation (9f40a82) I had to move all properties into of a model object into a single
$containerassociative array.I'm not particular satisfied with this approach either. Mostly because I don't like to refactor the internals of all models "just" for getting arrays working. And it makes it more difficult to figure out which type each property is (at least if thinking forward to PHP 7 and type hinting).
A positive side effect of this is that it will avoid bugs if someone decided to introduce a property named i.e.
attributeMapin their Swagger model as that would collide with our use of the static property$attributeMap.A lot of these problems originates from trying to fit everything into one
model.mustachetemplate. So another approach would be to use @wing328's approach in #2508 and split the template into several templates. That would require adding theisListContainerproperty mention above as well.Looking into this a question popped into my head. Why do we implement ArrayAccess for regular model objects at all? What is the use case for this except providing a way to avoid using the setters and gettes 😄
Tests
I added a test case testing whether arrays works as expected (7f99469).
The current pull request passes this test case and the existing ones as well (I have setup automatic test of the PHP tests for branches on my fork at: https://circleci.com/gh/arnested/swagger-codegen/47).