Adhere to strict PHP coding standards

[rawtaz]

Hi,

I recently tried out kryn.cms and came across a bunch of strict PHP errors/warnings, noted in issue #94.

These are not the errors/warnings listed, but I imagine you can use your IDEs to find which overriding functions have invalid signatures, for starters :) Here's a copy for reference though:

Strict standards: Declaration of Core\om\BaseDomainPeer::getFieldNames() should be compatible with BasePeer::getFieldnames($classname, $type = 'phpName') in /private/var/tmp/kryn-37dd/propel-classes/Core/om/BaseDomainPeer.php on line 25

Strict standards: Declaration of Core\om\BaseDomainPeer::translateFieldName() should be compatible with BasePeer::translateFieldname($classname, $fieldname, $fromType, $toType) in /private/var/tmp/kryn-37dd/propel-classes/Core/om/BaseDomainPeer.php on line 25

Strict standards: Declaration of Core\om\BaseDomainPeer::doDelete() should be compatible with BasePeer::doDelete(Criteria $criteria, PropelPDO $con) in /private/var/tmp/kryn-37dd/propel-classes/Core/om/BaseDomainPeer.php on line 25

etc..

Thanks!

[jaugustin]

could you provide the schema.xml ? of this model

[marcj]

Kryn.cms uses for the highlighted code above following schema: https://gist.github.com/4580332

[jaugustin]

@MArcJ this is your issue ;) https://github.com/KrynLabs/Kryn.cms/blob/propel1.6/core/PropelBasePeer.class.php#L5

you extends BasePeer but propel never do that because BasePeer is only an utility class with only static methods/properties

@willdurand you can close the issue ;)

[marcj]

@jaugustin well, actually not. The issue is another here: the basePeer property on database and table should just be used to change the BasePeer::do* calls in model's peer classes. The documentation says something equal: basePeer instructs Propel to use a different SQL-generating BasePeer class (or sub-class of BasePeer) for all generated objects.

But, and I guess here is the real failure (since Kryn.cms does not use the first 'hidden feature' with the extending of peer classes), propel does /extend/ the model's peer class with the given basePeer property instead of just replacing all BasePeer::do calls with the basePeer value.

So we got with basePeer set:

abstract class BaseDomainPeer extends \Core\PropelBasePeer

and without a value there:

abstract class BaseDomainPeer

Of course, the actually job of basePeer is done very well, but it seems that the additionally class extending is a 'hidden feature' or in other words: it's a unexpected behaviour which results in Strict standards warnings.

[marcj]

I thought about it a bit again.

As in the documentation wrote different SQL-generating BasePeer class (or sub-class of BasePeer), the basePeer class can be a mother class of your custom BasePeer property class. Unfortunately, this is not that easy the case, since the peer class builder puts this BasePeer property class as mother class of model's peer class.

So if we define there a own custom class that extends from BasePeer (as suggested in the documentation line above) the Strict standards warnings will always appear except when we re-write all methods to fit the method signature (which is not very practical).

So IMHO there are two ways to fix this:

• Do not suggest the use of a php class extended from BasePeer in the documentation line of the BasePeer property. Or better: indicate that you have to overwrite all methods of BasePeer that are also in your model peer class to avoid such Strict standards warnings (not very practical).
• Remove extend <baserPeer-property> in the peer class builder.

I'd prefer solution two, but I don't know whether that extend has a further use that I don't know.

@jaugustin, @willdurand what do you say? I can make a PR if you agree.

[willdurand]

Thing is, it will probably be a BC break...

William Durand | http://www.williamdurand.fr

On Sun, Jan 20, 2013 at 11:27 PM, MArc J. Schmidt notifications@github.comwrote:

I thought about it a bit again.

As in the documentation wrote different SQL-generating BasePeer class (or sub-class of BasePeer), the basePeer class can be a mother class of your custom BasePeer property class. Unfortunately, this is not that easy the case, since the peer class builder puts this BasePeer property class as mother class of model's peer class.

So if we define there a own custom class that extends from BasePeer (as suggested in the documentation line above) the Strict standards warnings will always appear except when we re-write all methods to fit the method signature (which is not very practical).

So IMHO there are two ways to fix this:

• Do not suggest the use of a php class extended from BasePeer in the documentation line of the BasePeer property. Or better: indicate that you have to overwrite all methods of BasePeer that are also in your model peer class to avoid such Strict standards warnings (not very practical).
• Remove extend in the peer class builder.

I'd prefer solution two, but I don't know whether that extend has a further use that I don't know.

@jaugustin https://github.com/jaugustin, @willdurandhttps://github.com/willdurandwhat do you say? I can make a PR if you agree.

— Reply to this email directly or view it on GitHubhttps://github.com/propelorm/Propel/issues/572#issuecomment-12478885.

[jaugustin]

@MArcJ I think the best thing to do is to update the documentation to explain the limitation.

I don't understand why your custom BasePeer need to extends the BasePeer, because if you need to call BasePeer Methods just call them with BasePeer::do...(), no ?

Ps: in Propel 2 this will go away ;)

[marcj]

@jaugustin, it's because BasePeer:: is never gonna used when you change the basePeer property.

I need the extending of my custom basePeer class just because after the setting of the basePeer property the whole model's peer class calls then only my new basePeer class directly and since I only wanted to replace one method it is a good deal to extend from BasePeer class because it brings me all other required methods to the stage. :-P

Unfortunately, I have no solution to work around this issue. I need to build the select statement itself to inject then a complex sql clause on my own but also need to trigger all events attached via 'preSelect'. So by calling BasePeer::createSelectSql directly in my ORM-layer, I still have to call doSelectStmt of the model's peer class itself and have then to cancel this process through a statement check in my PropelBasePeer's doSelect to avoid useless PDO statement execution, à la:

public static function doSelect(\Criteria $criteria, \PropelPDO $con = null){

    if (self::getIgnoreNextDoSelect()){
        self::setIgnoreNextDoSelect(false);
        return;
    }

    return \BasePeer::doSelect($criteria, $con);
}

I can now build for all other do* methods kind of proxy methods here to have the correct method signature, but that looks kinda like a hack.

@willdurand, if it's true that Propel supports 5.2 then I believe this peer static class extending is totally useless, since late static binding had been first introduced in php 5.3 and since model's peer class does not extend per default from BasePeer there is no 'static::' call where one can overwrite a peer method, so it should have never been used by someone in a useful way without getting those strict warnings.

PS: Do you know when we may reckon with Propel 2(alpha) ? Do you need help somewhere?

[marcj]

I've just tried to overrite now all methods that has invalid signatures, but no luck: The methods are fundamentally different.

Well ok, it's de facto not possible to use the basePeer property. :-(