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

Jump to bottom

Added config option for custom tracking image #17879

Merged
merged 6 commits into from Sep 3, 2021
Merged

Added config option for custom tracking image #17879

merged 6 commits into from Sep 3, 2021

Conversation

bx80
Copy link
Contributor

@bx80 bx80 commented Aug 12, 2021 •
edited by sgiehl

Description:

fixes #2672 Custom image to replace default 1x1 GIF image

Added a new config option which may be used to specify a custom image to override the default 1x1 transparent gif.
Either a image file location (with full path) or a base64 image string may be used.

[Tracker] custom_image = /path/to/my_custom_image.png
or
[Tracker] custom_image = "iVBORw0KGgoAAAANSUhEUgAAAAUAAAAFCAIAAAACDbGyAAAACXBIWXMAAC4jAAAuIwF4pT92AAAAB3RJTUUH5QgLFiABlwQnpwAAABl0RVh0Q29tbWVudABDcmVhdGVkIHdpdGggR0lNUFeBDhcAAAAUSURBVAjXY/wjLMyABJgYUAGpfABbJQEsALGyNgAAAABJRU5ErkJggg=="

Two unit tests are included which set the config option for both an image file and base64 image string, then check the respective tracker responses against the original image data.

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

@sgiehl sgiehl added the Needs Review PRs that need a code review label Aug 12, 2021
sgiehl
sgiehl requested changes Aug 12, 2021
View reviewed changes
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

Hi @bx80,
thanks for your contribution!
I had a quick look at the code and left a couple of suggestions. Once PHPCS is fixed and tests are running through I will quickly test the functionality locally so we can merge that soon :-)

core/Tracker/Response.php Outdated Show resolved Hide resolved
core/Tracker/Response.php Outdated Show resolved Hide resolved
tests/PHPUnit/Unit/Tracker/ResponseTest.php Outdated Show resolved Hide resolved
bx80 and others added 3 commits August 13, 2021 10:47
@bx80 @sgiehl
Update core/Tracker/Response.php
fd2419c 
Improved brevity

Co-authored-by: Stefan Giehl <stefan@matomo.org>
@bx80 @sgiehl
Update core/Tracker/Response.php
78c5d32 
Improved brevity

Co-authored-by: Stefan Giehl <stefan@matomo.org>
@bx80 @sgiehl
Update tests/PHPUnit/Unit/Tracker/ResponseTest.php
7023444 
Remove unnecessary debug comment

Co-authored-by: Stefan Giehl <stefan@matomo.org>
@bx80
Copy link
Contributor Author

bx80 commented Aug 12, 2021

Thanks for the suggestions, much appreciated.

tsteur
tsteur reviewed Aug 12, 2021
View reviewed changes
Copy link
Member

@tsteur tsteur left a comment

Choose a reason for hiding this comment

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

Nice work, left a few comments to clarify

config/global.ini.php Show resolved Hide resolved
core/Tracker/Response.php Outdated Show resolved Hide resolved
core/Tracker/Response.php Outdated Show resolved Hide resolved
core/Tracker/Response.php Show resolved Hide resolved
@bx80
Added restricted list of supported mime types, automatically determin…
9e33889 
…e mime type for image strings, switched to using file_get_contents, is_file and is_readable for image file loading
@sgiehl
Copy link
Member

sgiehl commented Aug 16, 2021

@bx80 it seems the tests you have added are failing. See https://app.travis-ci.com/github/matomo-org/matomo/jobs/531164790#L735

@bx80
Copy link
Contributor Author

bx80 commented Aug 17, 2021

The tests were failing because of the image recreate commit I'd previous added, a recreated image wouldn't match the original source image. I've reverted image recreation change due to performance and so the tests should now pass.

@github-actions
Copy link
Contributor

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Aug 25, 2021
@sgiehl sgiehl removed the Stale The label used by the Close Stale Issues action label Aug 27, 2021
sgiehl
sgiehl approved these changes Aug 27, 2021
View reviewed changes
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

From my point of view this should be ready to merge. Guess it's kind of expected that configuring a broken inline image (like c4==) also serves this broken image.
@tsteur shall we merge this for 4.5?

@tsteur
Copy link
Member

tsteur commented Aug 29, 2021

@sgiehl could you create an FAQ on matomo.org about how to change the tracking image before merging it? If we send a broken image when misconfigured then we should mention this also in the FAQ. We would need to mention how they can test the image.

@sgiehl
Copy link
Member

sgiehl commented Sep 3, 2021

@tsteur created https://matomo.org/faq/how-to-change-the-tracking-image/

@sgiehl sgiehl added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Sep 3, 2021
@sgiehl sgiehl merged commit 7e0b0e6 into matomo-org:4.x-dev Sep 3, 2021
@sgiehl sgiehl added this to the 4.5.0 milestone Sep 3, 2021
@justinvelluppillai
Copy link
Contributor

Thanks @bx80!

@sgiehl sgiehl mentioned this pull request Sep 6, 2021
Merged
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tsteur tsteur tsteur left review comments

@sgiehl sgiehl sgiehl approved these changes

Assignees
No one assigned
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
Milestone
4.5.0
Development

Successfully merging this pull request may close these issues.

New INI config setting for Custom image to replace default 1x1 GIF image
4 participants
@bx80 @sgiehl @tsteur @justinvelluppillai