Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[6.x] Postgresql column precision declaration fix #29873

Merged
merged 2 commits into from
Sep 13, 2019
Merged

[6.x] Postgresql column precision declaration fix #29873

merged 2 commits into from
Sep 13, 2019

Conversation

chmv
Copy link
Contributor

@chmv chmv commented Sep 5, 2019

Hello!

Briefly:
Allows you to create columns without optional precision:
"column1" timestamp WITHOUT TIME zone NOT NULL,

In detail:

Now there is no way to create Date/Time column with undefined precision as described in 8.5.1:
https://www.postgresql.org/docs/11/datatype-datetime.html#DATATYPE-DATETIME-INPUT

type [ (p) ] 'value'
where p is an optional precision specification giving the number of fractional digits in the seconds field. Precision can be specified for time, timestamp, and interval types, and can range from 0 to 6. If no precision is specified in a constant specification, it defaults to the precision of the literal value (but not more than 6 digits).

For example this code:

        Schema::create('test', function (Blueprint $table) {
            $table->timestamp('column1');
            $table->timestampsTz('column2');
            $table->time('column3');
            $table->timeTz('column4');
        });

will create this sql request:

CREATE TABLE "test"
(
    "column1" timestamp(0) WITHOUT TIME zone NOT NULL,
    "column2" timestamp(0) WITH TIME zone NOT NULL,
    "column3" time(0) WITHOUT TIME zone NOT NULL,
    "column4" time(0) WITH TIME zone NOT NULL
)

There is no way to get rid of (0) in a SQL query. Because of 0 as a default value:
public function timestamp($column, $precision = 0)

In this patch, null precision can be specified.

        Schema::create('test2', function (Blueprint $table) {
            $table->timestamp('column1', null);
            $table->timestampTz('column2', null);
            $table->time('column3', null);
            $table->timeTz('column4', null);
        });

in result:

CREATE TABLE "test2"
(
    "column1" timestamp WITHOUT TIME zone NOT NULL,
    "column2" timestamp WITH TIME zone NOT NULL,
    "column3" time WITHOUT TIME zone NOT NULL,
    "column4" time WITH TIME zone NOT NULL
)

Full backward compatibility. Since the previous behavior generated an invalid SQL query in this case.

Thank you!

P.S. This is my first pull request. Please correct me if I made a mistake. Thanks.

@GrahamCampbell GrahamCampbell changed the title Postgresql column precision declaration fix [6.x]Postgresql column precision declaration fix Sep 5, 2019
@GrahamCampbell GrahamCampbell changed the title [6.x]Postgresql column precision declaration fix [6.x] Postgresql column precision declaration fix Sep 5, 2019
@driesvints
Copy link
Member

@chmv heya. Thanks for your first pr. Can you maybe also add some tests for these changes?

@driesvints
Copy link
Member

@staudenmeir any chance you can have a look at this? Thanks!

@chmv
Copy link
Contributor Author

chmv commented Sep 6, 2019

@chmv heya. Thanks for your first pr. Can you maybe also add some tests for these changes?

I'll try. How to send changes in the right way? Do I need to make a new pr or is there a way to add to this? Thank you!

@driesvints
Copy link
Member

@chmv you can push commits onto your branch and they'll be added to this pr.

@chmv
Copy link
Contributor Author

chmv commented Sep 6, 2019

@driesvints Thanks! Done.

@staudenmeir
Copy link
Contributor

Looks good to me.

I think it would be easier to read if we switch the two sides:

(isset($column->precision) ? "($column->precision)" : '')

@chmv
Copy link
Contributor Author

chmv commented Sep 7, 2019

I'm not sure if this is rightly. Of course, it's really more beautiful. My first variant was (! is_null ...), but it's ugly, so I switched it. But isset... There is an unexpected default value of 0... in other method... It might confuse someone...
Do I need to make changes to this pr?

@taylorotwell taylorotwell merged commit c2c5d6c into laravel:6.x Sep 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants