[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!

[boris-glumpler]

I might be wrong, but I think this was actually a breaking change for Postgres users. Before this change, when you specified $table->timestampsTz(null); in a migration you actually got a timestamptz column. After this PR was merged, you end up with a timestamptz(0) column, which shouldn't be used according to the Postgres Don't Do This page.

To get timestamptz columns you now need to put the following into a service provider:

PostgresBuilder::defaultTimePrecision(null);

[calebdw]

@boris-glumpler,

This added the necessary mechanics to be able to change the default precision on a per database basis. This was the sister PR to #51363 which never got merged.

If you want full precision, then you should explicitly use:

$table->timestampsTz(6);
// or this in a service provider
PostgresBuilder::defaultTimePrecision(6);

If there's enough interest in the linked PR then I might try to reopen that against 13.x.

Regarding this being a BC: this was merged into master prior to the 12.0 release, so while it's unfortunate that there was a change in behavior in this particular edge case, such a change is perfectly acceptable.

If it helps, here's a PHPStan architecture test I wrote to enforce that 6 must be used as the precision, I also have another test that prevents any of the non-Tz methods from being used.

PHPStan Architecture Test

<?php

declare(strict_types=1);

namespace Tests\Architecture\Rules;

use Illuminate\Database\Schema\Blueprint;
use Override;
use PhpParser\Node;
use PhpParser\Node\Expr\MethodCall;
use PhpParser\Node\Identifier;
use PhpParser\Node\Scalar\LNumber;
use PHPStan\Analyser\Scope;
use PHPStan\Rules\Rule;
use PHPStan\Rules\RuleErrorBuilder;
use Tests\Architecture\Concerns\CanGetArg;

/** @implements Rule<MethodCall> */
final class PostgresTimestampsWithTimezoneRule implements Rule
{
    use CanGetArg;

    /** @var list<string> */
    private array $methods = ['dateTimeTz', 'softDeletesTz', 'timestampTz', 'timestampsTz'];

    #[Override]
    public function getNodeType(): string
    {
        return MethodCall::class;
    }

    #[Override]
    public function processNode(Node $node, Scope $scope): array
    {
        $errors = [];

        $calledMethods = $node->name instanceof Identifier
            ? [$node->name->toString()]
            : array_map(
                fn ($name) => $name->getValue(),
                $scope->getType($node->name)->getConstantStrings(),
            );

        foreach ($scope->getType($node->var)->getObjectClassReflections() as $reflection) {
            if (! $reflection->is(Blueprint::class)) {
                continue;
            }

            foreach ($calledMethods as $method) {
                if (! in_array($method, $this->methods, strict: true)) {
                    continue;
                }

                $position  = $method === 'timestampsTz' ? 0 : 1;
                $precision = $this->getArg($node, 'precision', $position)?->value;

                if ($precision instanceof LNumber && $precision->value === 6) {
                    continue;
                }

                $errors[] = RuleErrorBuilder::message('Timestamp precision must be 6.')
                    ->identifier('db.timestampPrecision')
                    ->build();
            }
        }

        return $errors;
    }
}

<?php

declare(strict_types=1);

namespace Tests\Architecture\Concerns;

use PhpParser\Node\Arg;
use PhpParser\Node\Expr\CallLike;

/**
 * This can be removed after functionality is added to the CallLike class.
 * @see https://github.com/nikic/PHP-Parser/pull/1089
 */
trait CanGetArg
{
    /**
     * Retrieves a specific argument from the given call-like node.
     *
     * Returns the named argument that matches the given `$name`, or the
     * positional (unnamed) argument that exists at the given `$position`,
     * otherwise, returns `null` for first-class callables or if no match is found.
     */
    private function getArg(CallLike $node, string $name, int $position): ?Arg
    {
        if ($node->isFirstClassCallable()) {
            return null;
        }

        foreach ($node->getArgs() as $i => $arg) {
            if ($arg->unpack) {
                continue;
            }

            if (
                $arg->name?->toString() === $name
                || ($i === $position && $arg->name === null)
            ) {
                return $arg;
            }
        }

        return null;
    }
}

You can then add this to your phptstan config via:

rules:
  - Tests\Architecture\Rules\PostgresTimestampsWithTimezoneRule

Best,

[boris-glumpler]

@calebdw, I didn't say it wasn't acceptable, just that it was a BC.

I don't actually recall reading anything about it, but I might've missed it. Tbh, I just want a regular timestamptz column without any precision. This is from the Postgres Wiki:

Don't use a precision specification, especially not 0, for timestamp columns or casts to timestamp.

[calebdw]

There wasn't anything in the upgrade docs because I didn't know about this particular case.

There is no difference in postgres between timestamptz and timestamptz(6)

[boris-glumpler]

There is no difference in postgres between timestamptz and timestamptz(6)

Until Postgres introduces nanoseconds 😉

[calebdw]

Lol, well we can cross that bridge if it ever happens (no need to prematurely optimize).

If I reopen the linked PR then we could set the default precision to null instead of 6 if that helps