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.

Sign up for GitHub

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

Allow empty excludedReferrers on the global setting #19834

Merged
merged 6 commits into from Oct 14, 2022
Merged

Allow empty excludedReferrers on the global setting #19834

merged 6 commits into from Oct 14, 2022

Conversation

peterhashair
Copy link
Contributor

Description:

Fixes: #19833

allow empty excludedReferrers on the global setting

Also, I notice when the setting changed successfully, there is no notification popup, should we add one?

Review

allow emply excludedReferrers
883e290 
allow emply excludedReferrers
@peterhashair peterhashair added this to the 4.12.1 milestone Oct 9, 2022
sgiehl
sgiehl requested changes Oct 10, 2022
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.

Might maybe be worth adding a test case for storing an empty value, to ensure the solution really works.

plugins/SitesManager/API.php Outdated Show resolved Hide resolved
plugins/SitesManager/API.php Show resolved Hide resolved
add some tests for set refer and update isLookLikeUrl function
c1c527c 
add some tests for set refer and update isLookLikeUrl function
@peterhashair
Copy link
Contributor Author

peterhashair commented Oct 10, 2022 •
edited

@sgiehl that makes more sense, add a couple of tests, just wondering example a should isLooksLikeURl accept that?

Peter added 2 commits October 11, 2022 10:04
update phpcs
fb13c9f 
update phpcs
revert Urlhelper, implement in api
bf34dfa 
revert Urlhelper, implement in api
sgiehl
sgiehl requested changes Oct 11, 2022
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.

@peterhashair example a might not be a valid record but we need to take care to be not too restrictive, as this might produce new problems.

plugins/SitesManager/API.php Outdated Show resolved Hide resolved
remove filter_var tests
c4c0b8a 
remove filter_var tests
@peterhashair peterhashair added 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. labels Oct 13, 2022
sgiehl
sgiehl approved these changes Oct 14, 2022
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.

I've fixed the code indent, that was wrong after your changes. Looks good now.

@sgiehl sgiehl merged commit 13c828a into 4.x-dev Oct 14, 2022
@sgiehl sgiehl deleted the m19833 branch October 14, 2022 11:28
@justinvelluppillai justinvelluppillai changed the title allow empty excludedReferrers on the global setting Allow empty excludedReferrers on the global setting Oct 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@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.12.1
Development

Successfully merging this pull request may close these issues.

Allow empty excludedReferrers on the global setting
2 participants
@peterhashair @sgiehl