CodersCare / l10nmgr

GNU General Public License v3.0
0 stars 9 forks source link

Handling of inline relations with parent tables other than tt_content #12

Open adamkoppede opened 1 year ago

adamkoppede commented 1 year ago

We use multiple inline relations in our installation with the most complex configuration being:

Content Element
    => inline child record of table A
    => inline child record of table B
        => inline (grand) child record of table A

Since the introduction of $GLOBALS['TYPO3_CONF_VARS']['EXTCONF']['l10nmgr']['inlineTablesConfig'] it is possible to use it in combination with tca select fields to configure both the Content Element => table A and Content Element => table B relations, but not the table B => table A relation.

In order to use l10nmgr with the table B => table A relation, we implemented a patch which is applied in this minimal reproduction example: https://github.com/adamkoppede/l10nmgr-inline-relations-example

Is there currently a solution within l10nmgr without patching? If not, could l10nmgr offer such ways to implement relations as described above?

mkarulin commented 7 months ago

I have another issue that your patch fixes when translating new inline elements of already translated record Example: I have tx_products_product record in EN that has inline "features"

$GLOBALS['TYPO3_CONF_VARS']['EXTCONF']['l10nmgr']['inlineTablesConfig'] = [
    'tx_products_domain_model_feature' => [
        'parentField' => 'product',
        'childrenField' => 'features',
    ],
]; 

Initially i can translate the product and its features without any problems.

But when i add a new feature to EN and export-import the XML i get no new localization in DE features.

@adamkoppede patch fixed this issue.

$GLOBALS['TYPO3_CONF_VARS']['EXTCONF']['l10nmgr']['additionalInlineTablesConfig'] = [
    'tx_products_domain_model_feature' => [[
        'parentTable' => 'tx_products_product',
        'parentField' => 'product',
        'childrenField' => 'features',
]];

What i would change in the patch is to avoid having 2 configuration arrays and we need someone from the core team to take a look at this as well.

mkarulin commented 7 months ago

@adamkoppede in your patch, what is the purpose of the additionalInlineTablesConfig for container? Is is it not for non tt_content tables only?

if (\TYPO3\CMS\Core\Utility\ExtensionManagementUtility::isLoaded('container')) {
        $GLOBALS['TYPO3_CONF_VARS']['EXTCONF']['l10nmgr']['additionalInlineTablesConfig']['tt_content'] ??= [];
        $GLOBALS['TYPO3_CONF_VARS']['EXTCONF']['l10nmgr']['additionalInlineTablesConfig']['tt_content'][] = [
            'parentTable' => 'tt_content',
            'parentField' => 'tx_container_parent'
        ];
    }
adamkoppede commented 7 months ago

@adamkoppede in your patch, what is the purpose of the additionalInlineTablesConfig for container? Is is it not for non tt_content tables only?

if (\TYPO3\CMS\Core\Utility\ExtensionManagementUtility::isLoaded('container')) {
        $GLOBALS['TYPO3_CONF_VARS']['EXTCONF']['l10nmgr']['additionalInlineTablesConfig']['tt_content'] ??= [];
        $GLOBALS['TYPO3_CONF_VARS']['EXTCONF']['l10nmgr']['additionalInlineTablesConfig']['tt_content'][] = [
            'parentTable' => 'tt_content',
            'parentField' => 'tx_container_parent'
        ];
    }

EXT:container (which the project I created this patch for uses together with EXT:l10nmgr) brings a relation between tt_content:tx_container_parent (child side) and tt_content:uid (parent side). EXT:l10nmgr had special handling for EXT:container before the patch, but with the patch I removed it in favor of re-using the implementation behind the $GLOBALS['TYPO3_CONF_VARS']['EXTCONF']['l10nmgr']['additionalInlineTablesConfig'] configuration (which was added in the patch).

While the comment there says that EXT:container should handle the child-parent relation fine during localization without the additional configuration, EXT:l10nmgr wouldn't know that the child is localized. The patch sets this information in https://github.com/adamkoppede/l10nmgr/commit/dc2a7c26127ba6ca47544e59f3edbe7aa2a7814f#diff-359f9307c6bf9eef5257ae636d08d3742f5de120d43d287dabe8b9417a40f347R245. For the records within the $implicitlyTranslatedRecords/$this->childMappingArray array, EXT:l10nmgr searches for the localization after all data handler operations are completed in https://github.com/adamkoppede/l10nmgr/commit/dc2a7c26127ba6ca47544e59f3edbe7aa2a7814f#diff-97ef693984ce13fc3de8a8a58cf575f51d90b24cee1ca3801f2977a09b1ed414L883-L909.

I introduced $GLOBALS['TYPO3_CONF_VARS']['EXTCONF']['l10nmgr']['additionalInlineTablesConfig'] in the patch because we encountered both of the following limitations of $GLOBALS['TYPO3_CONF_VARS']['EXTCONF']['l10nmgr']['inlineTablesConfig'] in said project:

It should be possible to use $GLOBALS['TYPO3_CONF_VARS']['EXTCONF']['l10nmgr']['inlineTablesConfig'] for the EXT:container configuration. This would however block the array-key for any other relations with tt_content as child table, which is unlikely to be an issue, since you'll probably only use a single extension/relation for nesting content elements.

$GLOBALS['TYPO3_CONF_VARS']['EXTCONF']['l10nmgr']['additionalInlineTablesConfig'] isn't limited to relations without tt_content on the parent side. It covers everything that can be configured with $GLOBALS['TYPO3_CONF_VARS']['EXTCONF']['l10nmgr']['inlineTablesConfig']. I left inlineTablesConfig intact, because editing it in a non-breaking way isn't possible in a way, that leads to a reasonable configuration interface, and removing it would be a breaking change, which would have broken my own usages of it.

The patch is only intended as a workaround for the duration of this discussion. This is also the reason why this is a issue instead of a pull request. In my opinion, the additional configuration isn't a good way moving forward, since I found myself adding all relations to it, after encountering issues with relations I forgot to add the configuration for. The information about all relations could be inferred from $GLOBALS['TCA'], since EXT:l10nmgr already requires a properly configured TCA on both sides.