@robocoder opened this Issue on February 13th 2012 Contributor

This is a planning/discussion ticket:

Background:

  • Piwik uses Reflection to expose a REST API.
  • Piwik uses phpdoc tags (e.g., @param), but these do not influence runtime behaviour.

Example from Actions/API.php:

        /**
         * Returns the list of metrics (pages, downloads, outlinks)
         * 
         * <a class='mention' href='https://github.com/param'>@param</a> int $idSite
         * <a class='mention' href='https://github.com/param'>@param</a> string $period
         * <a class='mention' href='https://github.com/param'>@param</a> string $date
         * <a class='mention' href='https://github.com/param'>@param</a> string $segment
         */
        public function get( $idSite, $period, $date, $segment = false, $columns = false)

Gaps:

  • we don't use the type hinting in @param for error messages or validating the API method's parameters
  • when it gets tricky, we see developers fall back to using Piwik_Common::getRequestVar()
  • it can't accommodate adding/removing/renaming a parameter without breaking backward compatibility
  • Controller methods are not similarly exposed
  • the Reflection API has overhead; why don't we cache metadata?

Proposal:

  • make use of annotations to actually influence runtime behaviour
  • use the Addendum library; it's LGPL and it doesn't require php 5.3
  • annotate a class or method

Phase 1:

  • feature would be disabled by default
  • adds annotation-based security
  • default to all if no @Access specified
  • allow all methods if no @Method is specified
  • unspecified @Param is passed through without validation/filtering

Phase 2 ("secure by default", i.e., break 3rd party plugins that haven't been updated):

  • feature is enabled and cannot be disabled
  • default to no-one if no @Access specified
  • disallow all methods if no @Method is specified
  • unspecified @Param is discarded
  • validated/filterd parameters are injected via $this->request (instead of our wrapper around $_GET/$_POST)

Possibilities:

Applies to class or method:

  • @Access("SUPER") - is superuser
  • @Access({"VIEW", "ADMIN", "SUPER"}) - has at least VIEW access

Applies to class or method:

  • @Method("GET") - restrict access to this method to HTTP GET
  • @Method({"GET", "POST"}) - restrict access to this method to HTTP GET or POST

Applies to all methods of a class:

  • @Param(idSite={assert="int", default=1}) - optional idSite; defaults to 1
  • @Param(referrerUrl={assert=@Type\LocalUrl}) - required referrerUrl must be a valid local URL, validated by Piwik_Annotation_Type_LocalUrl
  • @Param(newSiteUrl={filter=@Filter\Url}) - required newSiteUrlis filtered by Piwik_Annotation_Filter_Url

For methods, either use the @Param annotation above, or apply to the preceding @param for a method using:

References:

@mattab commented on February 17th 2012 Member

I can tell you have doing Symfony stuff ;)

It is a nice idea and concept, but i'm glad you put it in "Feature requests". I think the current API is quite easy to use.

when it gets tricky, we see developers fall back to using Piwik_Common::getRequestVar()

Never should Piwik_Common::getRequestVar() be used in an API, or it is a design fault. If you see it in core we should fix it for sure.
Technically the API proxy does the job of requesting the value and passing it to the API functions. Never it should directly call getRequestVar or it means the API will not work well in all cases.

it can't accommodate adding/removing/renaming a parameter without breaking backward compatibility

Current design accomodates this, because the parameter are not ordered when using new Piwik_API_Request() or when using the REST http API.

Ordering only matters when using directly the Php call ::getInstance()->method() but this method is only used for speed by the core algorithms, which can be easily changed if the API signature would need to change.

This Issue was closed on August 5th 2012
Powered by GitHub Issue Mirror