@diosmosis opened this Pull Request on April 9th 2021 Member

Description:

Refs #17428

Includes new test case and changes to make it pass. See inline comments for individual changes/fixes.

Review

  • [ ] Functional review done
  • [ ] Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • [ ] Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • [ ] Security review done see checklist
  • [ ] Code review done
  • [ ] Tests were added if useful/possible
  • [ ] Reviewed for breaking changes
  • [ ] Developer changelog updated if needed
  • [ ] Documentation added if needed
  • [ ] Existing documentation updated if needed
@diosmosis commented on April 9th 2021 Member

FYI @tsteur

@tsteur commented on April 11th 2021 Member

@diosmosis can you have a look at the failing test they might be related to this? If they fail because of this PR I will have another look after fixing it

@diosmosis commented on April 12th 2021 Member

@tsteur relevant tests should pass now

@diosmosis commented on April 12th 2021 Member

in line 113 should it set $this->params->setArchiveOnlyReport($requestedReport) or maybe setArchiveOnlyReport() should set setIsPartialArchive automatically?

I don't think it's technically necessary to set it, but it makes sense to be thorough. setArchiveOnlyReport() shouldn't call setIsPartialArchive(), that's called specifically by Archiver's when they handle the requestedReport parameter, letting ArchiveWriter know it only archived a single report.

I thought about unsetting it in that method, but we never re-use Parameters objects anywhere else (and shouldn't in normal uses), so it seemed like it could cause strange side effects.

do we ever need to reset the original/previously set IsPartialArchive value maybe? haven't checked yet where it's reused

In that specific line of code, no, because that's the start of the prepareArchive() method. In fact in that line of code we don't need to call the method there. I'll remove that.

would we need to back up the previously set value and restore it below in line 198?

Don't need to since we archive VisitsSummary before other plugins. We could do it, though, just in case something changes in the future.

@tsteur commented on April 12th 2021 Member

Don't need to since we archive VisitsSummary before other plugins. We could do it, though, just in case something changes in the future.

I reckon that might be useful just in case

@diosmosis commented on April 12th 2021 Member

@tsteur applied pr feedback

@tsteur commented on April 12th 2021 Member

fyi @diosmosis there are now some merge conflicts from merging the other PR

@diosmosis commented on April 12th 2021 Member

@tsteur :+1: just fixed, we'll see if the build passes

@tsteur commented on April 12th 2021 Member
@diosmosis commented on April 12th 2021 Member

@tsteur it was never really needed, since Loader is given a new Parameters instance just before using it. So it's already in a clean state.

This Pull Request was closed on April 12th 2021
Powered by GitHub Issue Mirror