Implement a "store in session" settings for forms #8245
Conversation
| $platform = $this->connection->getDatabasePlatform(); | ||
| $schemaManager = $this->connection->createSchemaManager(); | ||
| $fromTable = $schemaManager->introspectSchema()->getTable('tl_form'); | ||
| $toTable = clone $fromTable; | ||
| $toTable->addColumn('storeSession', Types::BOOLEAN, ['default' => false]); | ||
|
|
||
| $tableDiff = $schemaManager->createComparator()->compareTables($fromTable, $toTable); | ||
|
|
||
| foreach ($platform->getAlterTableSQL($tableDiff) as $query) { | ||
| $this->connection->executeQuery($query); | ||
| } |
There was a problem hiding this comment.
We should only run the specific ALTER TABLE query that is required for the migration. Any other changes should be shown to the user on the CLI so they can decide whether or not they want to apply them.
There was a problem hiding this comment.
That will not work in this case.
- We want to leave the setting disabled by default.
- For any existing forms need to enable the setting.
- For the migration to know when to run it determines whether the
storeSessionfield has already been added or not. - If the field was not added, it will add the field and set it to enabled.
- If the field was already added, the migration will not run.
There was a problem hiding this comment.
All of that is already implemented. You just have to replace lines 48 to 58 with a simple ALTER TABLE … statement as we do in other migrations:
There was a problem hiding this comment.
The code you referenced runs this ALTER TABLE, nothing else. I deliberately did not use a direct statement due to issues like #7985.
There was a problem hiding this comment.
Then I cannot merge the PR, I'm afraid. A concept change requires previous discussion and must be implemented for all migrations, not just for one.
There was a problem hiding this comment.
#8165 did not need compareTables() as it creates a new table using Doctrine DBAL. This PR on the other hand adds a new column to an existing table using Doctrine DBAL.
This PR and #8165 are the same in the sense that they are both using Doctrine DBAL schemas in order to alter the database, rather than executing ALTER TABLE or CREATE TABLE manually.
There was a problem hiding this comment.
Personally I do not want to look up / remember what statement I need to execute when I need to create a column with the configuration ['type' => 'boolean', 'default' => false] for example. Ideally the database abstraction layer should take care of this.
There was a problem hiding this comment.
Personally, I will need to look up what the DBAL abstraction notation is, I don't need to look up SQL for that 😂
There was a problem hiding this comment.
Using the DBAL abstraction it will always automatically use the correct type. Imagine if MySQL introduces a native boolean type and DBAL implement this for that platform. If we don't use the abstraction this migration will execute a different statement than our DCA migration will do, since the DCA uses the abstraction and this migration would not.
Uh oh!
There was an error while loading. Please reload this page.
A recent question/discussion on Slack whether and how long Contao forms store their data in the session made me think: I rarely ever use the
{{form_session_data::*}}insert tags (or previously{{post::*}}). Yet, we always store the form data in the session, albeit briefly.Thus I was thinking: wouldn't it make sense to only do this, when required? If you do not need this then your vanilla Contao instance might never need a session (and thus CSRF token) cookie in the front end at all.
This PR adds such a setting, similar to
tl_form.storeValues.It also adds a migration so that all existing forms have this setting enabled, for BC.
For new forms you then only enable this setting, if you need the data from this form in a
{{form_session_data::*}}insert tag.