Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Destructive DB schema update removes typeField from RecordTypes #188

Open
ri-klein opened this issue Apr 29, 2024 · 6 comments
Open

Destructive DB schema update removes typeField from RecordTypes #188

ri-klein opened this issue Apr 29, 2024 · 6 comments
Labels
Discussion Gather different opinions on a topic

Comments

@ri-klein
Copy link

Hello,
I noticed a weird behaviour when calling the CLI command database:updateschema destructive (from helhum/typo3-console). The command wants to delete the typeField from a custom multi-type RecordType (if the table is auto-generated by content-blocks). This does not happen when calling the CLI command immediately after flushing all caches; Only after caches have been built (e.g. after executing a page request).

The issue can be reproduced with e.g. the diver/instructor multi-type record example from the documentation (tested on TYPO3 12.4.14 with content-blocks 0.7.5):

  • Flush caches
  • Immediately call typo3 database:updateschema destructive -v --dry-run
  • Nothing happens
  • Call typo3 database:updateschema destructive -v --dry-run again
  • Removing the type field should be on the schema update list

I did some research on why that happens: During the execution of the CLI command, the event TYPO3\CMS\Core\Database\Event\AlterTableDefinitionStatementsEvent gets fired, which causes \TYPO3\CMS\ContentBlocks\Generator\SqlGenerator to get invoked.

Here, TYPO3\CMS\ContentBlocks\Definition\Factory\TableDefinitionCollectionFactory::createUncached gets called, which in turn calls TYPO3\CMS\ContentBlocks\Definition\Factory\ContentBlockCompiler::compile.

Immediately after flushing caches, $GLOBALS['TCA'] does not contain a definition for the RecordType; After building the caches however, it does contain a definition. \TYPO3\CMS\ContentBlocks\Definition\Factory\ContentBlockCompiler::prepareYaml then treats the typeField as an existing field (as \TYPO3\CMS\ContentBlocks\Schema\SimpleTcaSchemaFactory::has then wrongly returns true).

The resulting mergedTableDefinitions for ['TABLENAME']['fields']['TYPE_FIELD']['config'] then contains useExistingField = true, which means that in \TYPO3\CMS\ContentBlocks\Definition\TableDefinition::createFromTableArray, when \TYPO3\CMS\ContentBlocks\Definition\SqlColumnDefinitionCollection::createFromArray gets called, the column gets skipped and is missing from the table definition... which then causes database:updateschema destructive to remove the column.

I hope this helps you a bit on narrowing down the cause of the issue.

@jonaseberle
Copy link
Contributor

I think due to how ContentBlock definitions are cached we always need to do typo3 cache:flush before relying on the schema.

@nhovratov
Copy link
Contributor

Yeah, the database updateschema command must never use cached TCA! The DB Analyzer is always uncached and thus the call in SqlGenerator is always correct. I think this also applies to extension:setup. So, not a Content Blocks bug per se, but something for helhum/console.

@nhovratov
Copy link
Contributor

Created an issue for Helmut. Let's see what he says.

@helhum
Copy link

helhum commented Apr 29, 2024

Yeah, the database updateschema command must never use cached TCA!

Can you elaborate why?

The DB Analyzer is always uncached

This is an implementation detail.

From my understanding TCA should be built competently first, then SQL schema is derived from TCA (and additional providers)

@helhum
Copy link

helhum commented May 2, 2024

Yeah, the database updateschema command must never use cached TCA! The DB Analyzer is always uncached and thus the call in SqlGenerator is always correct.

Can you please elaborate (either here or in the console ticket) why loading TCA cached or uncached can lead to different results?

I think this also applies to extension:setup.

\TYPO3\CMS\Extensionmanager\Command\SetupExtensionsCommand is a TYPO3 core command using cached TCA. The only reason the bug is not triggered with this command, is that it does not perform destructive SQL operations.

@helhum
Copy link

helhum commented May 2, 2024

From the code comments in ContentBlockCompiler:

As such, this class does not have knowledge about real TCA, only about the YAML definition and how to transform it into the internal object format

This makes sense to me. However TCA is evaluated. I still don't understand why.

@nhovratov nhovratov added the Discussion Gather different opinions on a topic label Aug 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Gather different opinions on a topic
Projects
None yet
Development

No branches or pull requests

4 participants