statamic / eloquent-driver

Provides support for storing your Statamic data in a database, rather than flat files.
https://statamic.dev/tips/storing-content-in-a-database
MIT License
104 stars 74 forks source link

Fix form submission creation #122

Closed hotmeteor closed 1 year ago

hotmeteor commented 1 year ago

Uses the existing ID or null, allowing for incremental ID for new records.

Resolves #121

what-the-diff[bot] commented 1 year ago
jasonvarga commented 1 year ago

Hey Adam, I can't reproduce the original linked issue. Maybe it's been fixed already somehow. But I'd rather not add a change that has no visible benefit, even if it's tiny. If you can provide a way to reproduce it in a demo repo, that'd be great.

Thanks!

potsky commented 4 months ago

Hi @jasonvarga, we run with the same issue and the had same solution on Statamic 4!

We cannot rely on $this→id() because it calls this method in Statamic vendor/statamic/cms/src/Forms/Submission.php :

    /**
     * Get or set the ID.
     *
     * @param mixed|null
     * @return mixed
     */
    public function id($id = null)
    {
        return $this->fluentlyGetOrSet('id')
            ->getter(function ($id) {
                return $this->id = $id ?: str_replace(',', '.', microtime(true));
            })
            ->args(func_get_args());
    }

Given that the submission is a new model, the generated ID is a float based on the timestamp 🤯 which is something like 1715759899.6885 and the SQL query is :

select
  *
from
  "form_submissions"
where
  "form_submissions"."id" = '1715759899.6885'
limit
  1

leading to this exception : SQLSTATE[22P02]: Invalid text representation: 7 ERROR: invalid input syntax for type bigint: "1715759987.5163" CONTEXT: unnamed portal parameter $1 = '...' (Connection: pgsql, SQL: select * from "form_submissions" where "form_submissions"."id" = 1715759987.5163 limit 1)

How did it work even once?

Passing $this→id instead of $this→id() is a small modification but it really solve the problem because with a database we do not want to pass an id when creating record to let the database generate an incremented ID !

You told in issue #121 that you cannot reproduce the bug but how could you make it work? With the actual code of Statamic, it is impossible to make it work 🤔 There is certainly something we are missing... I can provide an access to our repository to test if you want. Tell me!

ryanmitchell commented 4 months ago

@potsky can you change your id column to not be an integer - would that not solve the issue?

potsky commented 4 months ago

@potsky can you change your id column to not be an integer - would that not solve the issue?

Yes 👏 Changing the int8 to a float8 works and the fun fact is that inserted IDs are still incremented integers!

I thing that you use MySQL and @hotmeteor and I are using PostgreSQL. PostgreSQL seems to check the column type before inserted anything even if when it will be inserted, it will not respect the wanted value and will generate an incremented int!

Here is my added migration for PostgreSQL if it can help:

<?php

use Statamic\Eloquent\Database\BaseMigration as Migration;

return new class extends Migration {
    public function up()
    {
        if ($this->isPostgresqlDatabase()) {
            DB::statement('ALTER TABLE "public"."form_submissions" ALTER COLUMN "id" SET DATA TYPE float8');
        }
    }

    public function down()
    {
        if ($this->isPostgresqlDatabase()) {
            DB::statement('ALTER TABLE "public"."form_submissions" ALTER COLUMN "id" SET DATA TYPE int8');
        }
    }

    private function isPostgresqlDatabase(?string $connection = null): bool
    {
        return $this->getDatabaseDriver($connection) === 'pgsql';
    }

    private function getDatabaseDriver(?string $connection = null): string
    {
        $key = \is_null($connection) ? Config::get('database.default') : $connection;

        return strtolower(Config::get('database.connections.'.$key.'.driver'));
    }
};
andreas-eisenmann commented 4 months ago

@potsky same problem here with PostgreSQL, my issue has already been linked.