@Findus23 opened this Pull Request on February 12th 2019 Member

Another PR that doesn't add, but mostly removes a feature. This time it is the getKeywordsForPageUrl widget. Why I want to argue that it can be completly deleted:

52624303-bce7b600-2eae-11e9-84ae-0f324ea3759b.png
  • as an example how to use the API, it should be rather a blog post or FAQ entry, instead of something permanently on the dashboard
  • I'm not sure how useful just printing keywords on the website is. I wouldn't be surprised if search engines have caught on a long time ago and just ignore them.
  • the two copies of the code (one for the preview and on for the text) aren't identical any more as fixes have been applied to just one of them
  • It is broken since Piwik 2.15 as it suggests using Common::sendHeader which obviously won't work when copying it into one's website: https://github.com/matomo-org/matomo/commit/65353d7bac8a90bf05545c0ecd693d81b684f9e2#diff-69b25f970e74caa20603c448a82972d4R329
  • and most importantly: No one has reported this in the last 3 years (at least I couldn't find a GitHub issue)

Deleting this widget is also not a backward incompatible change as the getKeywordsForPageUrl API it uses still exists and therefore people can continue to use their old code.

@tsteur commented on February 12th 2019 Member

Sounds good to me. Wanted to have this removed in the past as well but don't remember the reasons it wasn't done. May need to check if some developer docs point to it and if things are documented there as well.

@Findus23 commented on February 25th 2019 Member

Tests should be fixed now. (Will check after CI ran)
Not sure about removing the API as it would be a breaking change.

@sgiehl commented on February 25th 2019 Member

Maybe we should at least deprecate it and mark it as to be removed in Matomo 4 ?

@Findus23 commented on February 25th 2019 Member

@sgiehl Done.
Is adding the <a class='mention' href='https://github.com/deprecated'>@deprecated</a> as in https://github.com/matomo-org/matomo/pull/14093/commits/d74199bafe82216bbc67ad40f2333b1c84310c63 and adding to the changelog as in https://github.com/matomo-org/matomo/pull/14093/commits/11301a68ef5ac6f80a85da7c513172cf97263192 enough?

@sgiehl commented on February 25th 2019 Member

we use to write the version it will be removed in the code comment and in the changelog, so we know when it can be safely removed. e.g. in changelog: API Referrers.getKeywordsForPageUrl and Referrers.getKeywordsForPageTitle have been deprecated and will be removed in Matomo 4.0.0

and php comment

/**
 * <a class='mention' href='https://github.com/deprecated'>@deprecated</a> will be removed in Matomo 4.0.0
 */
@Findus23 commented on February 25th 2019 Member

Makes sense :heavy_check_mark:

@sgiehl commented on February 26th 2019 Member
@sgiehl commented on February 27th 2019 Member

fixed the test. Will merge if all related tests are passing now

This Pull Request was closed on February 28th 2019
Powered by GitHub Issue Mirror