PHPDoc: add ModelCriteria methods to Query classes for fluid interface #593

Open
rozwell opened this Issue Feb 4, 2013 · 19 comments

Comments

Projects
None yet
3 participants
Contributor

rozwell commented Feb 4, 2013

Currently when using autocompletion in Query classes, methods like select(), withColumn(), keepQuery(), setFormatter(), etc. the chain breaks (they return ModelCriteria) because of lack of PHPDocs.

I will create a PR for this, when I find some time to work on a fix.

Note: also, when using sluggable behavior, remove @method findOneBySlug since it's already declared as an actual method.

Member

staabm commented Feb 4, 2013

👍

Member

staabm commented Feb 16, 2013

just checked this... in the current trunk all the methods you mentioned have the right phpdocs and also method chaining works in my IDE (Zend Studio 10). Could you provide more context or check again with the latest Propel version?

I haven't checked sluggable.

Owner

marcj commented Feb 25, 2013

@rozwell, as staabm mentioned, the current trunk should be fine. About which version are we talking in your case?

Contributor

rozwell commented Feb 25, 2013

Sorry @staabm, I somehow missed your comment.

propel$ git log -1
commit a511f3cb4f68011f4b4627a54245bb700a97bb86
Merge: c2f9a82 82aa016
Author: William Durand <william.durand1@gmail.com>
Date:   Sat Feb 16 08:46:31 2013 -0800

    Merge pull request #611 from staabm/issue608

    select() + withColumn() query error

Here is the example for autocompletion:

$books = BookQuery::create()
  ->setFormatter(ModelCriteria::FORMAT_ON_DEMAND) // this one returns ModelCriteria instead of desired BookQuery
  ->orderByTitle() // so in here we won't be able to use autocompletion for BookQuery anymore
  ->find();

EDIT: checked in PhpStorm and NetBeans

Member

staabm commented Feb 25, 2013

@rozwell the method setFormatter resides in the ModelCritiera class, which is a base-class for all *Query classes. To fix this issue we would either need to overwrite all methods of ModelCriteria to re-define the phpdoc or we need to generate a more precise @method .. class comment int the BaseXXXQuery classes.

The second option seems to be better, because it doesn't introduce new and unnecessary methods.
In case we would introduce more @method ... phpdocs we should take care we don't miss any other method.

EDIT: hmm @rozwell could you try to change the phpdoc return typoe of ModelCriteria::setFormatter() to self.. maybe this would already do the trick...

Member

staabm commented Feb 25, 2013

Just checked the idea mentioned in the EDIT section of my last comment... did work for me in Zend Studio.

So instead of fixed classnames using "self" would be a solution... @rozwell please confirm.

OLD

    /**
     * Sets the formatter to use for the find() output
     * Formatters must extend PropelFormatter
     * Use the ModelCriteria constants for class names:
     * <code>
     * $c->setFormatter(ModelCriteria::FORMAT_ARRAY);
     * </code>
     *
     * @param  string|PropelFormatter $formatter a formatter class name, or a formatter instance
     * @return ModelCritieria The current object, for fluid interface
     *
     * @throws PropelException
     */
    public function setFormatter($formatter)

NEW

    /**
     * Sets the formatter to use for the find() output
     * Formatters must extend PropelFormatter
     * Use the ModelCriteria constants for class names:
     * <code>
     * $c->setFormatter(ModelCriteria::FORMAT_ARRAY);
     * </code>
     *
     * @param  string|PropelFormatter $formatter a formatter class name, or a formatter instance
     * @return self          The current object, for fluid interface
     *
     * @throws PropelException
     */
    public function setFormatter($formatter)
Contributor

rozwell commented Feb 25, 2013

@staabm I like your idea but unfortunately it doesn't work in neither, PhpStorm nor NetBeans. So it might not work in many others.
So it looks like @method .. class comments in BaseXXXQuery is the way to go.

Owner

marcj commented Feb 25, 2013

So it looks like @method .. class comments in BaseXXXQuery is the way to go.

👍 I think the same here.

Member

staabm commented Feb 25, 2013

hmm I started searching the web and found out that some IDEs support @return $this some support @return static and some @return self.
see e.g. http://youtrack.jetbrains.com/issue/WI-8038#comment=27-266252 or http://stackoverflow.com/questions/5858031/phpdoc-and-late-static-or-dynamic-binding

@rozwell could you please check which notation would work for you?

@method ... would be the most work therefore I want to check the alternatives first :-). Also it might be misleading because a classblock comment of a subclass would not match the method-block comment of the actual implementation.. Don't know yet how IDEs resolve such a "conflict"

Member

staabm commented Feb 25, 2013

per spec the only correct form would be @return self http://www.phpdoc.org/docs/latest/for-users/types, so I would love to support the spec and do it the "right way".

Member

staabm commented Feb 25, 2013

phpstorm supports this syntax also as of phpstorm6, http://blog.jetbrains.com/webide/2012/12/phpstorm-6-eap-build-124-373/

Contributor

rozwell commented Feb 25, 2013

Using latest PhpStorm 6: 126.260 and both: @return $this and @return static works there.
On NetBeans 7.3 (dev in unusable for php ATM) none of those three worked.

Member

staabm commented Feb 25, 2013

Since the standard supposes this syntax, I think we should adhere and do it that way..

@marcj @jaugustin any objections?

Owner

marcj commented Feb 25, 2013

Since we've always used the @method way, we should stick to it. As you mentioned, that (perhaps) 'new' spec @return self|$this etc is not that widely supported.

Contributor

rozwell commented Feb 25, 2013

What about @return ModelCriteria|$this|static to cover more editors?

Member

staabm commented Feb 25, 2013

ATM we use @method for magic __call() method autocompletion, but not for other methods ...

is @return ModelCriteria|$this|static supported in all major IDEs?

Owner

marcj commented Feb 25, 2013

yeah, wouldn't it be the fastest (and most compatible) way to extend those method list in the class' doc? We could actually also go both ways.

Member

staabm commented Feb 25, 2013

I will test both ways and than see which works best.

Owner

marcj commented Feb 25, 2013

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment