Skip to content

Conversation

@SentisMR
Copy link

@SentisMR SentisMR commented Feb 10, 2025

What

This PR allows passes the dateColumn through the Illuminate MySQL grammar wrapper so that you can use JSON columns. Fixes #71

image

Why

We have dates in JSON columns that we would like to use to trend. The "where_between" call already allows this but the "date_format" does not.

How

Simply call the MySqlGrammar->wrap on the column name instead of injecting the string directly into the date format call.

Copy link

@sarpavci sarpavci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to use MySqlGrammar you need to provide the connection information. You also need to add the Trend class.

laravel-trend/src/Trend.php

Lines 171 to 181 in 5ace11d

protected function getSqlDate(): string
{
$adapter = match ($this->builder->getConnection()->getDriverName()) {
'mysql', 'mariadb' => new MySqlAdapter(),
'sqlite' => new SqliteAdapter(),
'pgsql' => new PgsqlAdapter(),
default => throw new Error('Unsupported database driver.'),
};
return $adapter->format($this->dateColumn, $this->interval);
}

    protected function getSqlDate(): string
    {
        $connection = $this->builder->getConnection();
        $adapter = match ($connection->getDriverName()) {
            'mysql', 'mariadb' => new MySqlAdapter($connection),
            'sqlite' => new SqliteAdapter(),
            'pgsql' => new PgsqlAdapter(),
            default => throw new Error('Unsupported database driver.'),
        };

        return $adapter->format($this->dateColumn, $this->interval);
    }

namespace Flowframe\Trend\Adapters;

use Error;
use Illuminate\Database\Query\Grammars\MySqlGrammar;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
use Illuminate\Database\Query\Grammars\MySqlGrammar;
use Illuminate\Database\ConnectionInterface;
use Illuminate\Database\Query\Grammars\MySqlGrammar;

Comment on lines 21 to 23

$wrappedColumn = (new MySqlGrammar)->wrap($column);
return "date_format({$wrappedColumn}, '{$format}')";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$wrappedColumn = (new MySqlGrammar)->wrap($column);
return "date_format({$wrappedColumn}, '{$format}')";
$wrappedColumn = (new MySqlGrammar($this->connection))->wrap($column);
return "date_format({$wrappedColumn}, '{$format}')";

Copy link

@sarpavci sarpavci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or basically you can do:

laravel-trend/src/Trend.php

Lines 171 to 181 in 5ace11d

protected function getSqlDate(): string
{
$adapter = match ($this->builder->getConnection()->getDriverName()) {
'mysql', 'mariadb' => new MySqlAdapter(),
'sqlite' => new SqliteAdapter(),
'pgsql' => new PgsqlAdapter(),
default => throw new Error('Unsupported database driver.'),
};
return $adapter->format($this->dateColumn, $this->interval);
}

    protected function getSqlDate(): string
    {
        $adapter = match ($this->builder->getConnection()->getDriverName()) {
            'mysql', 'mariadb' => new MySqlAdapter(),
            'sqlite' => new SqliteAdapter(),
            'pgsql' => new PgsqlAdapter(),
            default => throw new Error('Unsupported database driver.'),
        };

        return $adapter->format(
            column: $this->builder->getGrammar()->wrap($this->dateColumn), // <--- HERE
            interval: $this->interval
        );
    }

@SentisMR
Copy link
Author

I think that the only place the connection gets used (in the mysql grammar anyways) is escaping raw sql and doing upserts. The code as written works in our environment. But your suggestion of grabbing the connection in the trend instead and pulling the builders grammar is cleaner. Thanks @sarpavci

@SentisMR SentisMR requested a review from sarpavci March 10, 2025 18:00
default => throw new Error('Invalid interval.'),
};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can checkout this file :)

@sarpavci
Copy link

@Larsklopstra Please take a look at this PR.

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.

json column as dateColumn not working

2 participants