Skip to content

Conversation

@SilasKenneth
Copy link
Member

@SilasKenneth SilasKenneth commented Jun 2, 2021

Summary

The getters for collection type properties are never creating the class objects in the right way. For instance, having the following data,

  ...
   "attendees": [
      {"email": "attendee1@example.com"},
      {"email": "attendee2@example.com"},
      {"email": "attendee3@example.com"}
   ]
  ...

the getters create one object passing the array of JSON objects as an argument.

Current behaviour.

   ...
   $this->_propDict['attendees'] = new Attendee($this->_propDict['attendees']);
   ...

Expected behaviour.

Instead, we would like to create an instance of Attendee for each attendee as follows,

 ...
  $attendees = [];
  foreach($this->_propDict['attendees'] as $attendee){
     $attendees[] = new Attendee($attendee);
  }
  $this->_propDict['attendees'] = $attendees;
 ...

Generated code differences

Screenshot 2021-06-03 at 10 07 21

Command line arguments to run these changes

Provide the command line arguments here so that reviewers can quickly repro the results of changes.

Links to issues or work items this PR addresses

Fixes #496
Fixes #87
Fixes #97
Fixes #115
Fixes #222
Fixes #76
Closes #523

Microsoft Reviewers: Open in CodeFlow

@SilasKenneth SilasKenneth force-pushed the silaskenneth/fix-collection-getters branch 5 times, most recently from ac336c0 to 792dccf Compare June 3, 2021 07:15
@SilasKenneth SilasKenneth force-pushed the silaskenneth/fix-collection-getters branch from 792dccf to 092c456 Compare June 3, 2021 07:37
@SilasKenneth SilasKenneth marked this pull request as ready for review June 5, 2021 04:14
@SilasKenneth SilasKenneth self-assigned this Jun 5, 2021
@SilasKenneth SilasKenneth requested a review from Ndiritu June 7, 2021 10:14
Ndiritu
Ndiritu previously approved these changes Jun 7, 2021
foreach ($this->_propDict['<#=camelCasePropertyName#>'] as $singleValue) {
$<#=camelCasePropertyName#> []= new <#=propertyTypeReference#>($singleValue);
}
$this->_propDict['<#=camelCasePropertyName#>'] = $<#=camelCasePropertyName#>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps something to think about in Kiota...
Should a getter be modifying/setting the value of a property? Maybe this logic should be handled within the constructor?/somewhere else during object creation/deserialization?

@SilasKenneth SilasKenneth force-pushed the silaskenneth/fix-collection-getters branch 3 times, most recently from 207684f to 096b6df Compare June 8, 2021 14:20
@SilasKenneth SilasKenneth added the Do Not Merge PR that are created but are not intended to be merged until some future condition is met. label Jun 8, 2021
@SilasKenneth SilasKenneth requested a review from Ndiritu June 9, 2021 07:02
@SilasKenneth SilasKenneth force-pushed the silaskenneth/fix-collection-getters branch from 096b6df to b04497a Compare June 9, 2021 15:17
@SilasKenneth SilasKenneth force-pushed the silaskenneth/fix-collection-getters branch from b04497a to cb464d7 Compare June 10, 2021 08:28
Copy link
Collaborator

@MIchaelMainer MIchaelMainer left a comment

Choose a reason for hiding this comment

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

We will want to batch these breaking changes with other breaking changes. We will need to capture information in an upgrade guide as customers will have taken a dependency on the previous broken behavior by implementing a workaround.

@Ndiritu
Copy link
Contributor

Ndiritu commented Sep 22, 2021

Changed the base branch of this PR to support/php-2.x.x which will contain all breaking changes and finally be merged to dev when v2 is stable

@Ndiritu Ndiritu merged commit eba2ba4 into support/php-2.x.x Sep 22, 2021
@Ndiritu Ndiritu deleted the silaskenneth/fix-collection-getters branch September 22, 2021 18:59
@Ndiritu Ndiritu restored the silaskenneth/fix-collection-getters branch September 22, 2021 18:59
@baywet baywet deleted the silaskenneth/fix-collection-getters branch March 8, 2022 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change Do Not Merge PR that are created but are not intended to be merged until some future condition is met.

Projects

None yet

4 participants