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

Check if plugin uses Piwik methods that are Deprecated or not API #6539

Closed
tsteur opened this issue Oct 28, 2014 · 23 comments
Closed

Check if plugin uses Piwik methods that are Deprecated or not API #6539

tsteur opened this issue Oct 28, 2014 · 23 comments
Assignees
Labels
c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. c: Tests & QA For issues related to automated tests or making it easier to QA & test issues. Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change.
Milestone

Comments

@tsteur
Copy link
Member

tsteur commented Oct 28, 2014

Plugins should only use methods that are declared in any plugins/*/API.php or any method or property tagged with @api.

We should check all PHP files within a plugin for any illicit method, constant or property usage and output any that is not public. If anything is used that is @deprecated a warning should be shown. If a plugin uses any method from another plugin (API.php) we should check whether the dependency is defined in require section of plugin.json.

This can be useful

https://github.com/nikic/PHP-Parser/ could be used for this which we already use in documentation generator.

@tsteur tsteur added the c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. label Oct 28, 2014
@mattab
Copy link
Member

mattab commented Oct 28, 2014

👍 this will be so useful!

Maybe you already mean this by "Automated test" but could we add an automated test that is executed in each plugin's build and that fails the plugin's build whenever the plugin uses a function that does is not tagged with @api ?

@tsteur
Copy link
Member Author

tsteur commented Oct 28, 2014

that's meant

@mattab mattab changed the title Check if plugin uses Piwik methods that are not API Check if plugin uses Piwik methods that are Deprecated or not API Oct 28, 2014
@mattab mattab added this to the Piwik 2.11.0 milestone Nov 3, 2014
@mattab mattab added Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change. c: Tests & QA For issues related to automated tests or making it easier to QA & test issues. labels Nov 3, 2014
@mattab mattab modified the milestones: Piwik 2.11.0, Piwik 2.12.0 Jan 8, 2015
@mattab
Copy link
Member

mattab commented Feb 19, 2015

Good news: work in progress in the phpstorm plugin: https://github.com/piwik/phpstorm-plugin-piwikstorm

Next steps is to run this in CI somehow. @diosmosis please write update here when you have one 👍

@diosmosis
Copy link
Member

Status: Need to get phpstorm running on travis. To do that, we need to register phpstorm on travis using our license file. Which is not straightforward. Can either use the license file or server (according to jetbrains), but not sure how.

@mattab
Copy link
Member

mattab commented Feb 26, 2015 via email

@diosmosis
Copy link
Member

Emailed jetbrains, waiting for a reply.

@mattab mattab modified the milestones: Piwik 2.13.0, Piwik 2.12.0 Mar 17, 2015
@mattab
Copy link
Member

mattab commented Mar 17, 2015

There is a possibility that what we want to do is not even possible... hopefully jetbrains support will be helpful and give us the solution, otherwise the work done so far may be "useless" (at least at automating the detection of deprecated code, we could still do it manually from time to time...)

@diosmosis diosmosis self-assigned this Mar 22, 2015
@diosmosis
Copy link
Member

Status: managed to get phpstorm license recognized in travis. Currently the process is killed before finishing.

@mattab
Copy link
Member

mattab commented Mar 23, 2015

Note that this code inspection ideally should run for:

  • Open source plugins at Piwik and Piwik PRO
  • Premium plugins at Piwik PRO
  • (optional: Third party plugins on the marketplace)

Maybe it will be necessary to add a new CI job to each build to run this code inspect?

@diosmosis
Copy link
Member

I think that would require a new CI job. Though it would be good if it didn't run on EVERY commit, just maybe master/develop branches. Or maybe for these repos we can setup our own server and add a github check (like travis/scrutinizer does) so travis doesn't get clogged.

@diosmosis
Copy link
Member

@mattab It looks like running the inspections can exhaust the memory, can we only run one build (not job, at least not yet) at a time on piwik-tests-plugins?

@mnapoli
Copy link
Contributor

mnapoli commented Mar 24, 2015

Aren't all Travis builds (even jobs) run into a separate virtual machine?

@diosmosis
Copy link
Member

You're right, I read the docs wrong. Not sure what to do though, I don't think it's possible to increase the memory.

@diosmosis
Copy link
Member

Status: inspections are created and uploaded as artifacts, ie, http://builds-artifacts.piwik.org/protected/LoginLdap.code_quality_tests/408.1/inspections/

TODO:

  • print summary of most important inspections out in travis console output
  • fix random failures (reduce max memory used by JVM?) (haven't seen a new one yet, crossing off for now)
  • fix bugs in new inspection (eg, Config::getInstance() results in failure because Singleton marks it as @api, not Config)
  • speed up code inspection build
  • make marketplace use new latest commit to base new builds off of (pull request created)
  • setup repo for core + pro plugins
  • setup weekly cron to test all maintained plugins for non-api/deprecated

@mnapoli
Copy link
Contributor

mnapoli commented Mar 24, 2015

Here it says the max memory builds can use is 3Gb if it helps http://docs.travis-ci.com/user/common-build-problems/#My-build-script-is-killed-without-any-error

@diosmosis
Copy link
Member

That's what I read ;)

@mattab
Copy link
Member

mattab commented Mar 25, 2015

What are the most little amount of work left to have it already useful in production?
we need to deliver small useful MVP and put it in CI already if possible and then we can improve the tool further in next sprint, as i'm afraid we don't have much more time for this sprint.

(for example for MVP I think we don't need the UI/JS part simply we could read Travis log output to see which file+line calls which deprecated / non-api function)

@diosmosis
Copy link
Member

Weekly report of maintained plugins' use of non-@api and @deprecated symbols is now in place & pull request for using it in piwik-tests-plugins via marketplace exists. Closing.

Follow up issue here: #7569

@mnapoli
Copy link
Contributor

mnapoli commented Mar 29, 2015

@diosmosis that's interesting :) where will those reports be? E.g. do you have a link to an example?

@diosmosis
Copy link
Member

If you look in the protected builds artifacts server you'll see folders like inspections.all or inspections.pluginname. Check those out (inspections.all is the most readable)

@mnapoli
Copy link
Contributor

mnapoli commented Mar 29, 2015

On http://builds-artifacts.piwik.org/? I can't find those folders in there (or in subfolders) :(

@diosmosis
Copy link
Member

Builds-artifacts.org/protected/. Doesn't get listed in directory ;)

@mattab
Copy link
Member

mattab commented Mar 29, 2015

Weekly report of maintained plugins' use of non-@api and @deprecated symbols is now in place & pull request for using it in piwik-tests-plugins via marketplace exists. Closing.

Nice progress! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. c: Tests & QA For issues related to automated tests or making it easier to QA & test issues. Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change.
Projects
None yet
Development

No branches or pull requests

4 participants