Strict Typing

[oojacoboo]

I looked around and couldn't find an issue opened for this. I'm sure it's been discussed though. What's the plan for typing the base classes, or offering the option to have strictly typed base classes? Not having the base classes strictly typed presents interfaces issue for us and I'm sure many others. PHP has moved much more in this direction and Propel is really holding us back.

[dereuromark]

You can make a PR to opt in this behavior. Sounds useful for many I am sure.

[dereuromark]

cc @nederdirk

[nederdirk]

I would love this, but I'm not sure if I have time to go ahead and implement this.

[dereuromark]

Maybe we can have a task force of several people? Helping out to get this solved for the next milestone release?

I see at least possibility of several steps

• [x] a) Adjust direct code base to be strict types as good as possible
• [x] b) Adjust code generation (generated PHP code) to be typed
• [ ] c) List and check left over pieces and why they cannot be typed, maybe there is also a change to refactor and improve the code to be typeable.

[oojacoboo]

I have a PHP8 compatibility branch that includes some typing updates to be in compatibility with PHP 8.1's typed interfaces. What branch should I target for this?

Additionally, I think we should focus on generated code with a priority over core lib code. The PR I have will cover the core lib for interface compatibility with typing, at least.

I have Propel running fine on 8.1 now.

[nederdirk]

Some thoughts on this:

• lib should definitely target the lowest common denominator, i.e. php7.4 currently
• for generated code it might be nice if the type signatures could be configured, i.e. a user of the Propel2 lib who targets php8.1 might benefit from generated type signatures with proper union types, where appropriate.

Other thoughts:

• in the past I have tried to add phpstan/psalm annotations (primarily related to generics-annotations to Query and Collection classes) which failed, mostly due to the BaseModelCriteria::$formatter property. I believe the array-formatter was the biggest problem, in combination with the addSelect method silently changing the formatter to SimpleArrayFormatter

[oojacoboo]

I've submitted PR #1805 which provides support for PHP 8 now. This is really only a step in the right direction. But, all tests are passing and everything is running well on ^8.0.

[dereuromark]

@oojacoboo I think we should for now keep at least last minor of 7.x as well. There is no good reason to jump too quickly. We can still add already all major types everywhere and clean up the API Wherever we use ...|false the API could be fixed to a cleaner nullable one of ?....

[dereuromark]

I started a PR https://github.com/propelorm/Propel2/pull/1821 but I do need help This is a lot, and quite some question marks in there as well regarding future API and what kind of types there should be.

[dereuromark]

Next PR is https://github.com/propelorm/Propel2/pull/1833 I also need a bit of help for that one.

[oojacoboo]

Where do we stand on this? It's actually really bad that you can't use proper interfaces with this lib due to these generated base classes. Interfaces end up being loosely typed, which makes it hard to really move a code-base forward. Being that strict typing is such a large part of PHP today and going forward, this just seems like a really important issue to me.

[mringler]

@oojacoboo Can you explain the issue you are having a bit? I am afraid I don't know what problem you are seeing.

[dereuromark]

It is also important to test/showcase this with latest version or better yet master, as there have been recent updates in that regard.

[oojacoboo]

@mringler it's pretty simple really. You cannot strictly type any interfaces that are implemented by a Propel model, at least for getters/setters defined in the base generated classes. That also limits the typing that can be done by other classes implementing that interface. It all leads back to Propel being stuck with loose typing.

[mringler]

@oojacoboo Do you mean the ActiveRecord interface? I think that is the only interface implemented by model classes. Maybe you could give an example?

[oojacoboo]

@mringler Here is an example:

<?php

interface Widget 
{
    // Cannot properly type this interface
    public function getProduct(): Product;
}

class BaseThingy
{
    /**
     * This phpdoc is the issue - should use proper return typing, or offer the option for loose typing
     * 
     * @return Product
     */
    public function getProduct()
    {
        return new Product();
    }
} 

class Thingy extends BaseThingy implements Widget
{
}

class NonPropelThingy implements Widget
{

    // Cannot properly type this method
    public function getProduct(): Product
    {
        return new Product();
    }
}

Similar issues are had with setters.

[mringler]

Sorry, I don't understand what you are describing. Let's try one more time.

I don't think there are getters in Propel base model classes which themselves instantiate objects (return new Product()). Propel model classes have two kinds of getters. First are those that return model data, i.e. for a book with a title, the method in the base model class looks like this:

    /**
     * Return the string representation of this object
     *
     * @return string The value of the 'title' column
     */
    public function __toString(): string
    {
        return (string)$this->getTitle();
    }

That one looks ok to me, and I think those only return primitive data, so I assume that is not what your getProduct() is.

Then there are getters which resolve objects associated through a relation, like how books have an author. Those look like this:

    /**
     * Get the associated ChildAuthor object
     *
     * @param ConnectionInterface $con Optional Connection object.
     * @return ChildAuthor|null The associated ChildAuthor object.
     * @throws \Propel\Runtime\Exception\PropelException
     */
    public function getAuthor(?ConnectionInterface $con = null)
    {
        if ($this->aAuthor === null && ($this->author_id != 0)) {
            $this->aAuthor = ChildAuthorQuery::create()->findPk($this->author_id, $con);
            /* ...
             */
        }

        return $this->aAuthor;
    }

where ChildAuthor is an aliased type (not sure why, I don't think it is necessary):

use Propel\Tests\Bookstore\Author as ChildAuthor;

Do you mean those getters/setters which resolve relationships, and the problem is the type alias? It looks like you want to write an interface with the model methods, and that is not working? What error are you getting?

[oojacoboo]

@mringler You're looking way too deep into this. It's really not that complicated. Even methods like:

    /**
     * Get the associated ChildAuthor object
     *
     * @param ConnectionInterface $con Optional Connection object.
     * @return ChildAuthor|null The associated ChildAuthor object.
     * @throws \Propel\Runtime\Exception\PropelException
     */
    public function getAuthor(?ConnectionInterface $con = null)
    {
        if ($this->aAuthor === null && ($this->author_id != 0)) {
            $this->aAuthor = ChildAuthorQuery::create()->findPk($this->author_id, $con);
            /* ...
             */
        }

        return $this->aAuthor;
    }

Are problematic for interfaces. That whole passing of a ConnectionInterface is entirely Propel specific and cannot/should not be replicated in an interface. Therefore, it cannot/should not be used in other classes also implementing an interface.

The example I provide about returning a new Product doesn't matter. It could return anything. It was just an example. The issue is when implementing interfaces on Propel models.

Have you ever implemented interfaces on Propel models and used them? Or, have you tried re-implementing a method in a Propel model defined in the base class? Try strictly typing it - it won't work because the method signatures must match. And, because the base classes aren't strictly typed, you're stuck with loose typing.

[mringler]

Sounds like you want to write an interface which can be implemented by several classes, one of which is a propel models:

interface BookInterface{
     public function getAuthor(): AuthorInterface;
}

class PropelBook extends BasePropelBook implements BookInterface{
    ....
}

class SomeOtherBook implements BookInterface{
    public function getAuthor(): AuthorInterface
    {
        return ...
    }
}

This will not work if there is an Author relation on the book table, since Propel will generate methods on the base model class that are not compatible with the signature in BookInterface:

public function getAuthor(?ConnectionInterface $con = null) // method has an argument and no return type

Is the problem you run into?

[oojacoboo]

@mringler Correct, but it's not just relational getters, like in your example. It's an issue for all getters and setters, since they're loosely typed. And, you don't have control over these since they're in the base class.

PHP 7.4 is EOL next week too BTW. PHP 8.0 supports a lot of strict typing features. Most all code being written today is using strict typing - none of this phpdoc type hinting stuff anymore.

[mringler]

Ah, for some reason I got __toString() above, not the actual getter:

    /**
     * Get the [title] column value.
     * Book Title
     * @return string
     */
    public function getTitle()
    {
        return $this->title;
    }

Yes, that one does not have a return type. I think it could be added quite easily though.

But the argument in relational getters will probably never go away. Your quarrel there is with PHP, which does not let you override method signatures in child classes. I think you are going to have to write wrapper classes or fiddler around with the names (which is messy).

[oojacoboo]

My argument with relational getters is a design decision from Propel/ActiveRecord. It has nothing to do with overriding method signatures, if we really want to get down to it. Obviously overriding method signatures is well discussed. But, passing in a connection object is a mess of a design decision. I get that one is not going to go away anytime soon, or ever. That's fine. It is what it is.

However, for all other getters/setters, there is no reason why they cannot/should not be strictly typed at this point. Which is the entire purpose of this issue ticket - adding strict typing to the base classes.

[mringler]

Excellent, I finally understand what your post was about.

• You want to discuss if getters and setters can be fully typed, both input variables and return type.
• And you want to discuss if the ConnectionInterface argument can be dropped from relational getters.

As to the types in getters/setters, I think the reason why types were not added in the last iteration is backwards compatibility. If you feel this is no longer an issue, feel free to explain why and make an argument for changing it.

As to the ConnectionInterface argument, I am not really sure if there was a deeper reason to put it there except just exposing the arguments of the underlying call to the query object. Maybe someone else knows what problem it solves, again, feel free to ask and argue why it is there, why you think it should be removed and why it warrants breaking backwards compatibility.

[dereuromark]

I would also say that a PR to showcase the changes and also the implications on the system as a whole would be most productive moving foward. As the current open issues show, we won't progress much without actual changes being done.

[oojacoboo]

@mringler BC can be provided with a setting that allows people to stick with loosely typed phpdoc typehints instead of actual real typing. But typing base classes should absolutely be the default and expected behavior going forward.

As for the ConnectionInterface, yea, I don't know the reason for that other than convenience and lack of proper design planning. IMO, this could be set on the model object as a property through another setter, or possibly using a proxy method, like so:

$foo->useConnection($connection, 'getBars');

@dereuromark where is the logic for the base class generation handled? Do you know?

[dereuromark]

I dont I wonder if we can close this as we moved to strict types where possible now?

[oojacoboo]

@dereuromark Are we closing this and opening another issue ticket? I'm a little taken back by the lack of understanding for the need or the entire purpose of this ticket.

Some of the core code might have some stricter typing, an improvement. But the generated base classes are loosely typed with no option for strict typing.

[mringler]

If in the end I understood @oojacoboo correctly, I believe the open questions are

• if we are willing to break BC and add type declarations to the public methods of generated model and query classes
• if there is a downside to dropping the ConnectionInterface parameter from relational getters like $book->getAuthor($con) (which also breaks BC and probably tests)

And of course if somebody can do it (won't be me, I am out of time).

[dereuromark]

Is there a PR we can discuss that would enable this with a feature flag to "enable"? By default disabled? This way we could allow this to be added without a direct BC break to all existing apps. If they solve their issues, they can update to the strict version in their own time table by just switching the feature flag.

[mringler]

enable this with a feature flag

Yes, that is probably the best way to do it. Particularly changing signature of getters substantially breaks BC, as those are overridden quite frequently.

Removing the ConnectionInterface parameter might have consequences beyond breaking type declaration of overrides. The big question is how it impacts setups with multiple databases, and if it impacts how transactions work.

[oojacoboo]

@mringler if possible, we could provide a base model method to set the ConnectionInterface. That base method could potentially take a closure argument, allowing you to pass it for a given call, that way it's not adding to any state.

$foo->withConnection(fn (Foo $foo) => $foo->getBar(), $connection);

As for a PR, some direction would be helpful in knowing where these base classes are generated.

[mringler]

@oojacoboo Look at Propel\Generator\Builder\Om\QueryBuilder and Propel\Generator\Builder\Om\ObjectBuilder. Older methods use a rather convoluted inline format, newer ones use templates, which are located in ./templates/Builder/Om/. Query objects extend Propel\Runtime\ActiveQuery\ModelCriteria and its parent classes, where the base methods like filterBy() are declared.