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

Fix build failures #15016

Merged
merged 17 commits into from Oct 22, 2019
Merged

Fix build failures #15016

merged 17 commits into from Oct 22, 2019

Conversation

katebutler
Copy link

No description provided.

@@ -63,7 +63,7 @@ public function getApiForTesting()
'date' => '2010-01-01',
'periods' => 'year',
'testSuffix' => 'phpSerialized',
'format' => 'original',
'format' => 'php',
Copy link
Member

Choose a reason for hiding this comment

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

@katebutler as discussed we probably need to adjust instead the original handler to respect hideIdSubDatatable. The test was added to ensure when the dataTable object is serialized that no random information is being added see 46ed8c2 . But now an array is serialized and the actual logic is no longer tested. If possible be good to still use original and also add a comment that this test is testing the serialized DataTable


public function removeIdSubDataTables($response)
{
$response = preg_replace('/"subtableId";i:[\d]+;/', '', $response);
Copy link
Member

@tsteur tsteur Oct 20, 2019

Choose a reason for hiding this comment

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

@katebutler to possibly avoid regexes would it be better to do something similar as in PHP renderer where we iterate over all rows and then call in this case $row->removeSubtable() like

 if ($this->hide...) {
foreach ($table->getRows() as $row) {
   $row->removeSubtable();
}
 }

haven't tested but could work

@tsteur tsteur merged commit 6869ffc into 3.x-dev Oct 22, 2019
@tsteur tsteur deleted the kb-fix-build branch October 22, 2019 22:12
@mattab mattab added c: Tests & QA For issues related to automated tests or making it easier to QA & test issues. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. labels Oct 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: Tests & QA For issues related to automated tests or making it easier to QA & test issues. 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