Remove ObjectBuilder's hard dependency to Mysql #466

Closed
mpscholten opened this Issue Oct 9, 2013 · 7 comments

Comments

Projects
None yet
3 participants
Member

mpscholten commented Oct 9, 2013

    public function addTemporalAccessorComment(Column $column)
    {
        $script = '';
        $clo = $column->getLowercasedName();

        $dateTimeClass = $this->getBuildProperty('dateTimeClass');
        if (!$dateTimeClass) {
            $dateTimeClass = '\DateTime';
        }

        $handleMysqlDate = false;
        if ($this->getPlatform() instanceof MysqlPlatform) {
            if ($column->getType() === PropelTypes::TIMESTAMP) {
                $handleMysqlDate = true;
                $mysqlInvalidDateString = '0000-00-00 00:00:00';
            } elseif ($column->getType() === PropelTypes::DATE) {
                $handleMysqlDate = true;
                $mysqlInvalidDateString = '0000-00-00';
            }
            // 00:00:00 is a valid time, so no need to check for that.
        }

(taken from the ObjectBuilder)

You can see the instanceof which checks for the MysqlPlatform.
But this should be replaced with something like $platform->is...

This need to be solved for #465

Member

jaugustin commented Oct 9, 2013

@mpscholten what is the problem with the instanceOf ? if your platform is not MySQL this part is skipped.

Member

mpscholten commented Oct 9, 2013

First I you want to extend Propel with your own specific Platform and want to invoke the same functionality you cannot without extending MysqlPlatform. Maybe your ExamplePlatform also have the invalid string 0000-00-00 00:00:00 but you cannot tell this the Builder because the builder will check this himself (via instanceof).

Also this method is repeated all over the Builder class and because of DRY we shouldn't do that.

Owner

willdurand commented Oct 10, 2013

We could subclass builders for each platform, but I don't really see the point here. DRY does not apply here, it is a builder, it makes decisions regarding the things it has to build, that's it.

willdurand closed this Oct 10, 2013

Member

mpscholten commented Oct 10, 2013

Not sure if you understood me right:
I think we should replace the code with something like this:

if ($this->getPlatform() instanceof HasInvalidTemporalStringPlattform) {
            if ($column->getType() === PropelTypes::TIMESTAMP) {
                ...
                $.... = $platform->getInvalidTimestampString();
            } elseif ($column->getType() === PropelTypes::DATE) {
                ...
                $.... = $platform->getInvalidDateString();
            }
        }

So the builder doesn't care about a specific implementation, it cares about the interface. This would decouple the MysqlPlatform from the Builder.

Owner

willdurand commented Oct 10, 2013

Ah, so you would like to introduce new interfaces?

Member

mpscholten commented Oct 13, 2013

Yes, an interface or maybe better something like PlatformInterface::hasStreamBlobImpl().

if($column->getType() === PropelTypes::TIMESTAMP) {
    $invalidTemporalValue = $platform->getInvalidTimestampValue();
} elseif($column->getType() === PropelTypes::DATE) {
    $invalidTemporalValue = $platform->getInvalidDateValue();
}

And in the MysqlPlatform:

public function getInvalidTimestampValue() {
    return '0000-00-00 00:00:00';
}

And in all the other Platforms it would be

public function getInvalidTimestampValue() {
    return null;
}
Owner

willdurand commented Oct 13, 2013

ok

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