nhovratov / content-blocks

TYPO3 CMS Content Blocks - Content Types API
https://docs.typo3.org/p/contentblocks/content-blocks/main/en-us/
GNU General Public License v2.0
50 stars 14 forks source link

Destructive DB schema update removes typeField from RecordTypes #188

Open ri-klein opened 2 months ago

ri-klein commented 2 months ago

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):

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 commented 2 months ago

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

nhovratov commented 2 months ago

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 commented 2 months ago

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

helhum commented 2 months ago

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 commented 1 month ago

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 commented 1 month ago

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.