[12.x] feat: configure default datetime precision on per-grammar basis

[calebdw]

Hello!

Please see my previous PR (#51793) for context. This adds the following properties:

• New $connection and $grammar properties on Illuminate/Database/Schema/Blueprint.
• Illuminate/Database/Schema/Blueprint requires $connection in constructor
• New static $defaultTimePrecision property on Illuminate/Database/Schema/Builder.

Thanks!

[driesvints]

Just want your 👀 on this one if you have a sec @hafezdivandari.

[hafezdivandari]

We have Builder::$defaultStringLength property and Builder::defaultStringLength() method that have been used on Blueprint's char and string column types.

framework/src/Illuminate/Database/Schema/Builder.php

Lines 37 to 42 in ae63a5b

	/**
	* The default string length for migrations.
	*
	* @var int|null
	*/
	public static $defaultStringLength = 255;

I suggest to add Builder::$defaultTimePrecision = 0 property and Builder::defaultTimePrecision() and use it on all time types including dataTime and timestamp:

public function dateTime($column, $precision = null)
{
    $precision ??= Builder::$defaultTimePrecision;

    return $this->addColumn('dateTime', $column, compact('precision'));
}

No need to change anything on toSql method of the Blueprint class.

[calebdw]

@hafezdivandari,

I suggest to add Builder::$defaultTimePrecision = 0 property

This would mean that the precision is no longer configurable on a per grammar basis (just like the getDateFormat on the grammars) which is what this PR is attempting to provide.

I didn't like adding the logic in the toSql method either, but I needed the injected grammar---however, this can be cleaned up by injecting the connection via the constructor instead---given that this is targeting 12.x I feel like this is an acceptable bc which cleans up passing the connection / grammar around inside the class.

I've created a fixup commit (which can be squshed later) so you can preview the changes. If we don't like it then I can pop that commit off

[hafezdivandari]

Why per grammer and not per schema builder? You can do PostgresBuilder::defaultTimePrecision(6)?

[calebdw]

I originally put it on the grammar right underneath the getDateFormat method because they should be related and I'm looking to change the default Postgres precision for #51363 (in the framework, not in userland).

You can do PostgresBuilder::defaultTimePrecision(6)

That changes the default value for all the schema Builders, not just for Postgres:

[taylorotwell]

This feels like a lot of changes for such a simple change 🙃

[calebdw]

@taylorotwell,

Yeah 🫤, it was more involved than I would like. Most of this is the fallout from injecting the Connection/Grammar via the Blueprint constructor instead of the build method so that the datetime methods can have access to the Grammar when they are called. Otherwise I had to wait until the build method is called with the Grammar to loop over all the columns updating the ones with null values.

I can split the refactor (injecting the Connection/Grammar into the Blueprint constructor) into a separate PR if you like, or I can update if you have other ideas on a better way to do this (being able to configure the default precision on a per-grammar basis).

[hafezdivandari]

@calebdw

I originally put it on the grammar right underneath the getDateFormat method because they should be related and I'm looking to change the default Postgres precision for #51363 (in the framework, not in userland).

As I said on this comment, we already have properties with similar functionality on the Schema\Builder class. So it's a good practice to keep it consistent.

That changes the default value for all the schema Builders, not just for Postgres:

You may define Builder::$defaultTimePrecision like this:

class Builder
{
    public static $defaultTimePrecision = 0;
    
    public static function defaultTimePrecision(?int $precision)
    {
        static::$defaultTimePrecision = $precision;
    }
}

class PostgresBuilder extends Builder
{
    public static $defaultTimePrecision = 0;
}

Then use it like this:

PostgresBuilder::$defaultTimePrecision; // 0
MySqlBuilder::$defaultTimePrecision;    // 0

PostgresBuilder::defaultTimePrecision(6);

PostgresBuilder::$defaultTimePrecision; // 6
MySqlBuilder::$defaultTimePrecision;    // 0

Using connection:

$connection->getSchemaBuilder()::$defaultTimePrecision;

[calebdw]

Ah okay, but we'd still need the connection injected into the Blueprint constructor so we have access to it when the time methods execute

[hafezdivandari]

we'd still need the connection injected into the Blueprint constructor

Yes, and it may be considered as a useful change IMO - Having direct access to connection instance on Blueprint class instead of passing it to each method.

[calebdw]

@hafezdivandari, I've moved the precision to the builder

[hafezdivandari]

Thanks @calebdw, lgtm. But you may draft this until PR #51373 is merged, resolve conflicts on Blueprint then ready for review.

[calebdw]

@hafezdivandari, it's been a hot minute, but I finally got around to rebasing this after your changes made it to master

[hafezdivandari]

@calebdw sorry for late response, still lgtm, thanks.

PS: It may be better to summarize the changes on the PR description:

1. New $connection and $grammar properties on Illuminate/Database/Schema/Blueprint class.
2. New $defaultTimePrecision property on Illuminate/Database/Schema/Builder class.

[taylorotwell]

Thanks!

[calebdw]

Thank you sir!