Stop relying on Type::__toString by greg0ire · Pull Request #2835 · doctrine/dbal

greg0ire · 2017-09-02T17:44:46Z

It is a somewhat fragile thing to do.

greg0ire · 2017-09-02T17:47:19Z

lib/Doctrine/DBAL/Platforms/AbstractPlatform.php

-use Doctrine\DBAL\Event\SchemaAlterTableAddColumnEventArgs;
-use Doctrine\DBAL\Event\SchemaAlterTableRemoveColumnEventArgs;
-use Doctrine\DBAL\Event\SchemaAlterTableChangeColumnEventArgs;
-use Doctrine\DBAL\Event\SchemaAlterTableRenameColumnEventArgs;


I reordered the use statements. Is it good or did the previous order have some meaning I should keep?

No worries on reordering it, I usually do it on a separated commit to make it easier for people to review stuff 😄

Yeah, I got lazy there, I'll do that.

greg0ire · 2017-09-02T17:57:23Z

lib/Doctrine/DBAL/Platforms/AbstractPlatform.php

+                $type = $field['type'];
+                if ($type instanceof Types\IntegerType ||
+                    $type instanceof Types\BigIntType ||
+                    $type instanceof Types\SmallIntType


Should I introduce a parent class? This test is done several time ( it done in SQLServerPlatform, for instance)

Or maybe a marker interface?

Do you mean something like an AbstractNumberType? It wouldn't introduce number-specific functionality and just be an empty class, if you meant such a solution. A marker interface would have no methods, right? I'm not sure about new class or interface files, that are only introduced for checks. Do those have a purpose in userland for custom types or would those be only used internally?

I also considered (static?) methods for internal type checking, but those had to be maintained too and aren't elegant either. It's like we can only introduce more complexity to Doctrine.

Do you mean something like an AbstractNumberType? It wouldn't introduce number-specific functionality and just be an empty class, if you meant such a solution.

Correct, but indeed, an interface is better

A marker interface would have no methods, right?

yup

I'm not sure about new class or interface files, that are only introduced for checks. Do those have a purpose in userland for custom types or would those be only used internally?

People that introduce numeric types and would like to benefit from the code behind those checks could implement that interface, so yeah, there is a purpose in userland.

I also considered (static?) methods for internal type checking, but those had to be maintained too and aren't elegant either. It's like we can only introduce more complexity to Doctrine.

Yeah, and I think a marker interface is really not a big thing to make public.

👍 for the marker interface. I'd suggest to mark it as internal to tell people to simply no use it (@internal).

greg0ire · 2017-09-02T17:57:29Z

lib/Doctrine/DBAL/Platforms/AbstractPlatform.php

+                ) {
                    $default = " DEFAULT ".$field['default'];
-                } elseif (in_array((string) $field['type'], ['DateTime', 'DateTimeTz']) && $field['default'] == $this->getCurrentTimestampSQL()) {
+                } elseif (($type instanceof Types\DateTimeType || $type instanceof Types\DateTimeTzType)


Indeed, and we probably should have added the immutable type here as well

@lcobucci the immutable type extends the mutable type, so it's already ok :)

lcobucci

Let's move the build fixes out of this PR (I'm working on that) and simplify things using a marker interface.

greg0ire · 2017-09-08T12:09:07Z

The build fix was just to check I didn't actually break things. I will remove them when the PR looks like it's mergeable.

greg0ire · 2017-09-08T12:10:19Z

Also, please see #2838 for a quick fix of the build until you find a better solution if there is one.

greg0ire · 2017-09-09T08:08:40Z

lib/Doctrine/DBAL/Platforms/AbstractPlatform.php

-                } elseif ((string) $field['type'] == 'Date' && $field['default'] == $this->getCurrentDateSQL()) {
-                    $default = " DEFAULT ".$this->getCurrentDateSQL();
-                } elseif ((string) $field['type'] == 'Boolean') {
+                } elseif ($type instanceof Types\DateType && $field['default'] == $this->getCurrentDateSQL()) {


Maybe I should name the marker interface differently because of Types\DateType does not implement it although it does map to a PHP \DateTimeInterface instance? Suggestions?

greg0ire · 2017-09-09T08:09:31Z

@lcobucci please review again. I kept the build-fixing commit to ensure I did not break anything.

greg0ire · 2017-09-09T11:39:18Z

Let's move the build fixes out of this PR (I'm working on that)

How about merging #2838 right now and do it your way later? That would make things easier for all the outstanding PRs...

Majkl578

Needs some tests, as you can see, Travis is green even though there's a fatal error. :/

Majkl578 · 2017-09-09T13:58:18Z

lib/Doctrine/DBAL/Platforms/AbstractPlatform.php

-                    $default = " DEFAULT ".$this->getCurrentDateSQL();
-                } elseif ((string) $field['type'] == 'Boolean') {
+                } elseif ($type instanceof Types\DateType && $field['default'] == $this->getCurrentDateSQL()) {
+                    $default = " DEFAULT ".$this>getCurrentDateSQL();


Nice one (accidentaly removed -), one would say it would fatal right away, but it won't, only when executed... PHP...

woops, nice catch!

Really good eyes here. I didn't saw that too. 👍

I got very unlucky, that's the only if that was not covered.

greg0ire · 2017-09-09T15:44:22Z

I created #2851 to prove the test work with the previous code. Please review and merge it, and I shall rebase this.

greg0ire · 2017-09-09T15:46:13Z

Oh snap I need to add #2838 to it if I want to prove anything

Majkl578 · 2017-09-11T03:32:09Z

Related issue in ORM: doctrine/orm#5760

It groups things naturally.

It is a somewhat fragile thing to do.

greg0ire · 2017-09-11T06:17:48Z

Pushed again without the test commit, which was just merged. Please review again @Majkl578 @lcobucci

Ocramius

LGTM 👍

Ocramius · 2017-09-11T07:06:59Z

🚢

greg0ire · 2017-09-11T07:28:55Z

Please don't review again then :P

SenseException · 2017-09-11T11:00:44Z

lib/Doctrine/DBAL/Types/PhpIntegerMappingType.php

+
+namespace Doctrine\DBAL\Types;
+
+use Doctrine\DBAL\Platforms\AbstractPlatform;


What was the point for this use statement?

None I'm afraid 😓

SenseException · 2017-09-11T11:00:57Z

lib/Doctrine/DBAL/Types/PhpDateTimeMappingType.php

+
+namespace Doctrine\DBAL\Types;
+
+use Doctrine\DBAL\Platforms\AbstractPlatform;


What was the point for this use statement?

greg0ire commented Sep 2, 2017

View reviewed changes

greg0ire force-pushed the avoid_type_to_string branch from 2fe3fd6 to e291f88 Compare September 2, 2017 17:52

greg0ire commented Sep 2, 2017

View reviewed changes

greg0ire force-pushed the avoid_type_to_string branch 2 times, most recently from ed1e052 to 9008fdf Compare September 2, 2017 18:03

greg0ire changed the title ~~WIP: Stop relying on Type::__toString~~ Stop relying on Type::__toString Sep 2, 2017

greg0ire force-pushed the avoid_type_to_string branch 2 times, most recently from b5ea715 to eb705ad Compare September 2, 2017 22:58

greg0ire mentioned this pull request Sep 3, 2017

Refactor type system #2841

Open

9 tasks

lcobucci suggested changes Sep 8, 2017

View reviewed changes

greg0ire force-pushed the avoid_type_to_string branch from eb705ad to 6131a51 Compare September 9, 2017 08:06

greg0ire commented Sep 9, 2017

View reviewed changes

lcobucci mentioned this pull request Sep 9, 2017

Update build to use stages (adding PHPCS to check for CS violations) #2849

Merged

Majkl578 suggested changes Sep 9, 2017

View reviewed changes

greg0ire force-pushed the avoid_type_to_string branch from 6131a51 to c65275b Compare September 9, 2017 15:41

greg0ire force-pushed the avoid_type_to_string branch from c65275b to 2cdc601 Compare September 10, 2017 22:07

greg0ire added 2 commits September 11, 2017 08:16

Sort use statements

b66d704

It groups things naturally.

Stop relying on Type::__toString

9e75bb0

It is a somewhat fragile thing to do.

greg0ire force-pushed the avoid_type_to_string branch from 2cdc601 to 9e75bb0 Compare September 11, 2017 06:16

Ocramius approved these changes Sep 11, 2017

View reviewed changes

Ocramius self-assigned this Sep 11, 2017

Ocramius added the Improvement label Sep 11, 2017

Ocramius added this to the 2.7 milestone Sep 11, 2017

Ocramius merged commit ba421d1 into doctrine:master Sep 11, 2017

SenseException reviewed Sep 11, 2017

View reviewed changes

greg0ire deleted the avoid_type_to_string branch September 11, 2017 11:22

greg0ire mentioned this pull request Sep 11, 2017

Style improvements #2858

Merged

holtkamp mentioned this pull request Sep 12, 2017

CURRENT_TIMESTAMP not possible anymore as default value doctrine/orm#6698

Closed

Majkl578 mentioned this pull request Sep 13, 2017

DateTimeImmutable error with default value CURRENT_TIMESTAMP #2859

Closed

GwendolenLynch mentioned this pull request Oct 11, 2017

Custom Datetime db type incompatible with Doctrine bolt/bolt#7065

Closed

Majkl578 mentioned this pull request Aug 17, 2018

Remove Doctrine\DBAL\Types\Type::__toString() #3257

Merged

github-actions bot locked as resolved and limited conversation to collaborators Aug 14, 2022


		namespace Doctrine\DBAL\Types;

		use Doctrine\DBAL\Platforms\AbstractPlatform;

Uh oh!

Conversation

greg0ire commented Sep 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lcobucci left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg0ire commented Sep 8, 2017

Uh oh!

greg0ire commented Sep 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg0ire commented Sep 9, 2017

Uh oh!

greg0ire commented Sep 9, 2017

Uh oh!

Majkl578 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg0ire Sep 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg0ire commented Sep 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

greg0ire commented Sep 9, 2017

Uh oh!

Majkl578 commented Sep 11, 2017

Uh oh!

greg0ire commented Sep 11, 2017

Uh oh!

Ocramius left a comment

Choose a reason for hiding this comment

Uh oh!

Ocramius commented Sep 11, 2017

Uh oh!

greg0ire commented Sep 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

lcobucci left a comment •

edited

Loading

greg0ire Sep 9, 2017 •

edited

Loading

greg0ire commented Sep 9, 2017 •

edited

Loading