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

Do not serialize every property of Periods which can appear in DataTable metadata. #13280

Merged
merged 2 commits into from Aug 17, 2018

Conversation

diosmosis
Copy link
Member

Since subperiods are created recursively and the Translator has all translations, serializing both of these properties massively inflates the API response when using php serialization.

@mattab / @tsteur would like to get this in 3.6.0

@diosmosis diosmosis added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Aug 13, 2018
@diosmosis diosmosis added this to the 3.6.0 milestone Aug 13, 2018
@tsteur
Copy link
Member

tsteur commented Aug 13, 2018

Are we actually still supporting PHP serialisation? Does this have any side effects besides the API (probably not)? Be good to maybe have a test for this. No big opinion on the PR otherwise.

@diosmosis
Copy link
Member Author

Test for the changes here? Or for php serialized responses?

@tsteur
Copy link
Member

tsteur commented Aug 13, 2018

php serialized response

@tsteur
Copy link
Member

tsteur commented Aug 13, 2018

I just realize if this format is API it would be a breaking change in theory as you maybe wouldn't be able to use the output of 3.6.0 in an older version of Matomo, eg 3.5? Shouldn't be too big of an issue though but can be a problem. Really don't know how much we support that format.

@diosmosis
Copy link
Member Author

diosmosis commented Aug 13, 2018

Tested locally from 3.5.1 => "instance w/ these changes" and it seems to work, the objects inflate. Only thing that doesn't work is getting a pretty string using a period in the datatable metadata. Not sure what kind of effect that would have.

@diosmosis
Copy link
Member Author

^ CC @mattab

@mattab
Copy link
Member

mattab commented Aug 17, 2018

fyi we're planning to eventually deprecate this format in API responses: #8100

@mattab
Copy link
Member

mattab commented Aug 17, 2018

@diosmosis the builds are failing, can you please look into them? I'm merging anyway so we proceed with the RC release

@mattab mattab merged commit 46ed8c2 into 3.x-dev Aug 17, 2018
@mattab mattab deleted the period-serialization branch August 17, 2018 10:31
@diosmosis
Copy link
Member Author

@mattab if all the UI code is in angular & the PHP UI layer doesn't make API requests, then we won't need the original format. Otherwise it's the only way to proxy (that I can think of).

@mattab
Copy link
Member

mattab commented Aug 17, 2018

@diosmosis the issue #8100 is about removing format=php but we would still support format=original for internal API requests. Is there a problem with removing format=php do you think? if so please post a note in #8100

@diosmosis
Copy link
Member Author

No issue w/ removing format=php, just format=original

InfinityVoid pushed a commit to InfinityVoid/matomo that referenced this pull request Oct 11, 2018
…ble metadata. (matomo-org#13280)

* Do not serialize every property of Periods which can appear in DataTable metadata.

* Add test for original PHP serialized response.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants