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

[BUGFIX] Localization in flux:grid columns #1331

Closed
wants to merge 1 commit into from

Conversation

monosize
Copy link
Member

FluidTYPO3\Flux\Backend\Controller\LocalizationController

  • extended class of \TYPO3\CMS\Backend\Controller\Page\LocalizationController

AjaxRoutes to \TYPO3\CMS\Backend\Controller\Page\LocalizationController

  • flux_languages_page_colpos -> get available languages of the flux:grid column (step 2 of 4 in the localization process)
  • flux_records_localize_summary -> get the uid, icon and title off all elements which would localized ( step 3 of 4 in the localization process)
  • flux_records_localize -> build the cmdMap with the elements uids and own commands and additional flux specific data

FluidTYPO3\Flux\Backend\Preview

  • Localization.js is adjusted to work with flux:grid

FluidTYPO3\Flux\Backend\TceMain

  • processCmdMap to handle the localization for flux:grid elements
  • processCmdmap_postProcess to add Data in $pasteDataMap`

\TYPO3\CMS\Backend\Domain\Repository\LocalizationRepository

  • to handle flux related fields tx_flux_colum and tx_flux_parent

\TYPO3\CMS\View\PreviewView

  • extended the gridcolumn template with data-flux-column and data-flux-parent

resolves #1120

@monosize
Copy link
Member Author

The PHPUnit tests failed. Had no time to correct them. It's too late, good night ;-)

@NamelessCoder
Copy link
Member

Phew, this looks like way too much code to solve a bug (which by the way also exists in the TYPO3 core as far as I am aware). Though I'm all for solving bugs, I really don't think the right way to do it is to replace the entire controller, language resolving, ajax routing, JS and markup. There has to be a cleaner way which requires no overrides, or at least requires far fewer overrides?

Maybe we can re-use the IRRE localisation call as a custom action in our "localise" button? When you localise nested content using the IRRE field (note: workspaces must not be installed or the field isn't shown, but you can still save the field - it's just not added to TCA showitem) as far as I'm aware, that does it correctly?

Don't get me wrong, I very much appreciate the work you put into it, but we also have to maintain all this code, for multiple TYPO3 versions - so I'd much rather not override how the core does the localisation.

Also, my knowledge in this area is limited (from a user perspective as well) so I don't know all the little details about how it is expected to work and what's causing it to not work for our case. I wish I could help more, but right now, "perhaps reuse the IRRE localize logic?" is the best I can do.

@monosize
Copy link
Member Author

@NamelessCoder Thanks for your statement. Did you mean for the 'IRRE localize logic' this method \TYPO3\CMS\Core\DataHandling\DataHandler::inlineLocalizeSynchronize?
For that we have to change the FluidTYPO3\Flux\Backend\TceMain:: processCmdMap from this PR.

But we need here also own ajaxRoutes to fetch the correct informations for each flux:grid column.
In my opinion we need a Controller for the AjaxRoutes. We need a Repository to fetch the correct information for the flux:grid columns. And we need the adjusted Localization.js. Without this information we have no chance to localize a flux:grid column! on top of TYPO3v7 od v8.

I do not expect major changes to the localization structures for TYPO3v9. None of the extended Classes and used methods are marked as deprecated except the \TYPO3\CMS\Core\Database\DatabaseConnection Class which flux self use. And for TYPO3v9+ we have to migrate the whole FluidTYPO3 cosmos to Doctrine support.

The Localization Support is one of the big feature of TYPO3, so we have to bite the bullet ;-)
And with this changes it has a approximately lifetime of 3 years (TYPO3v8 LTS). Maybe longer...

@monosize monosize force-pushed the localization branch 7 times, most recently from 18b09f5 to d26b894 Compare February 18, 2017 14:07
@monosize
Copy link
Member Author

Since TYPO3v8.6 then new field 'l10n_source' stores the parent of the translated element.

Bugfixes in translate mode

  • create new content buttons are now hidden in flux:grid columns when the translate mode is used.
  • Missing untranslated elements in a flux:grid column are now determined correctly.

Bugfix in copy mode

  • The translate button does not appear in a flux:grid column if untranslated items are present.

Insert language mode detection in \FluidTYPO3\Flux\View\PreviewView
Now works also in TYPO3v7.6: using Localization.7.6.js

The PHPUnit tests are failing. They are corrected later.
Tested with TYPO3v8.6 and TYPO3v7.6.15

@NamelessCoder
Copy link
Member

Let's take a step back here - because this is absolutely massive as a bug fix. Basically we would be overriding the entire localisation logic and provide overrides for JS for two separate versions of TYPO3, and it all stems from a fairly simple problem.

To analyse the problem:

  1. TYPO3 isn't passing our information about Flux parent and column name. It does pass a colPos value, but it is not the correct colPos value (it is for example zero because the parent sits in page colPos zero).
  2. The colPos being passed therefore says nothing about "we're translating a grid column" and the translation fails.
  3. We are not doing any particular processing when localisation happens (we're basically just correcting sorting information).

Here's what I know about the above:

  1. We can solve the problem of an incorrect colPos being put in the action to localise. This shouldn't require JS - but we may need to override more of PageLayoutView. The colPos value we insert here, should be the session-stored array of target information identified by an integer larger than FLUX_COLPOS_CONTENT that is then looked up in the session on the receiving end. If we implement this I do expect something else to happen: DataHandler won't trigger localisation of the correct content elements (it will localise nothing at all). To overcome this, we need to catch the localisation command, unroll the session-stored target information, read a list of content elements and then dispatch localisation commands for each of those.
  2. The fact that the translation stalls on the last step may be caused by the TYPO3 core itself, in which case we will simply live with that (and consider contributing a fix for it). However, it may also be a side effect from the first problem above - because the wrong content elements get localised, the response is wrong or incomplete in the localisation wizard. It is somewhat likely that fixing the first problem also fixes this one.
  3. The extra processing we'd need to do, described in the first problem, could ideally sit next to where we correct the sorting values. There is also a small chance that fixing the first problem also fixes this one.

Specially about the third item:

There's a VERY odd side effect in TYPO3 regarding the "tt_content as IRRE" relation we use for tx_flux_children. The problem is that when you perform an action which generates a new version or a copy of a parent record, children do get processed as well - but they end up with odd sorting numbers. Rather than being generously spaced like tt_content sorting values normally are, they become perfectly sequential (and not in the order of the original sorting value).

This may be playing into the result you see after a copy operation and it definitely affects records you copy/paste in workspaces mode. I've attempted to correct that problem by having Flux copy the sorting number from the original child record to the new copy, after an operation which caused the children to have sequential sorting numbers. I'm working on a fix for that - just wanted to let you know that it happens when you trigger one of these actions ;)

So I think our solution to this should be a lot smaller than this suggested patch. Our goal: get the right colPos value transferred, extend the hook that catches the localisation action and perform selective localisation on whichever children are resolved based on session-stored target info array identified by colPos value.

Hope this makes sense!

@monosize
Copy link
Member Author

The main problem is, that the TYPO3 core fetches in Localization.js the content elements which should translated via ajax. The core AjaxRoutes are languages_page_colpos, records_localize_summary and records_localize. There is no hook to extend the associated methods in \TYPO3\CMS\Backend\Controller\Page\LocalizationController
which calls methods in \TYPO3\CMS\Backend\Domain\Repository\LocalizationRepository
so we need here definitely own AjaxRoutes to an Flux related LocalizationController FluidTYPO3\Flux\Backend\Controller\LocalizationController

flux_languages_page_colpos -> get available languages of the flux:grid column (step 2 of 4 in the localization process)
flux_records_localize_summary -> get the uid, icon and title off all elements which would localized ( step 3 of 4 in the localization process)
flux_records_localize -> build the cmdMap with the elements uids and own commands and additional flux specific data

Without these own LocalizationController and Repository we have no chance.

@NamelessCoder
Copy link
Member

NamelessCoder commented Feb 20, 2017

The core AjaxRoutes are languages_page_colpos, records_localize_summary and records_localize

As far as I can tell, these all call LocalizationController which delegates the resolving of records to LocalizationRepository. The one exception is the "perform localization" method which generates the command map, but for that one the following should be true:

  • It receives a list of UIDs from the request which means if the right UIDs are sent, we wouldn't need to override the method.
  • It does not need to include Flux fields in the update command since it works on a preset list of UIDs.
  • If we need post-processing we have that in our DataHandler hook (to fix position in localisation).

This means that assuming we fix the resolving of colPos value, the controller will receive the right value and pass it to the LocalizationRepository.

Which should mean we need two concrete things:

  1. Fixing the resolving of colPos value that Localization.js does (we probably need a change in PreviewView for this, that's okay - but it should definitely use the session-stored colPos values!).
  2. Override LocalizationRepository methods to replace the queries that get performed.

Sadly, 2. requires different implementations for LTS and 8.x (with Doctrine) but there's nothing to do about that.

If we can limit the scope of this change to those two things I would have no objections to it. I will then follow that up with a FEATURE request for the TYPO3 core to add signals which manipulate the Doctrine query before it gets dispatched, and hope to get that in TYPO3v9 so we can use that instead of our overrides some day.

@NamelessCoder
Copy link
Member

Correction: we may not need two different overrides of LocalizationRepository. Assuming the method signatures are the same, we can simply catch our colPos-higher-than-FLUX_COLPOS_CONTENT case and do our special queries, and if colPos is lower, call method on parent.

@monosize
Copy link
Member Author

That's sound good. I'll try it and build a version with colPos-higher-than-FLUX_COLPOS_CONTENT.

@monosize
Copy link
Member Author

monosize commented Mar 2, 2017

@NamelessCoder Hi Claus, the usage with colPos-higher-than-FLUX_COLPOS_CONTENT works great for an adjusted \TYPO3\CMS\Backend\Domain\Repository\Localization\LocalizationRepository

  • but we need here a LegacyVersion :-( because \TYPO3\CMS\Backend\Domain\Repository\Localization\LocalizationRepository::getRecordsToCopyDatabaseResult uses in 8.6+ a \Doctrine\DBAL\Driver\Statement result and in 7.6 it's a mysqli_resultresult.

  • We can now use the original TYPO3 version from Localization.js ;-)

  • This version use a changed $pasteDatamap. My effort to change the DataHandler->datamap was not successful see comments in ed3b93a#diff-bbfb8daa54a85bb162203489e904ae37R343

I think there are some bugs in the TYPO3 localization:
we have an element which were localized in 'copy' mode. Localizing it to another language with the 'translate' mode, so the l18n_parent is 0 - that is illogical. I think l18n_parent must become the uid of this element, because it's the original.

Here are some important informations for TYPO3v8.6+

@monosize monosize force-pushed the localization branch 2 times, most recently from 119e763 to 2939387 Compare March 2, 2017 17:49
@monosize
Copy link
Member Author

monosize commented Mar 2, 2017

I pushed a new version which also has been tested in TYPO3 7.6.
Now ready for 7.6 and 8.6

Correct handling of localized elemnts in flux grid columns
@mithri
Copy link

mithri commented Mar 17, 2017

Hi,

i`ve tested this commit on master. Translation is working fine now (finally, thanks!!!!). But there are some hiccups, for example the translation window is not closing.

I`d like to see this bugfix merged and published to the TER, even if its not the cleanest solution. Is there anything i can do to help? More testing?

Thanks!
Tobias

Copy link
Member

@NamelessCoder NamelessCoder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't forgotten, but because of the amount of overrides this would require, I decided to look for another possibility. So I've asked if it would be possible to add signals in TYPO3 itself in the places where we currently require class overrides - and I've asked if those could be included in TYPO3 LTS which we will require for Flux soon after it is released.

Will update when I have more to tell. With a bit of luck the suggested signals will be approved and we can avoid all overrides this PR requires. If that can't happen I'll reconsider these overrides.

@Kleisli
Copy link

Kleisli commented Oct 18, 2017

Hi @NamelessCoder

Are there any news on this one?

Thanks for all your work!
Till

@michael-binder
Copy link

Hey,

have you found another possibility for this issue?

Thanks!
Michael

@NamelessCoder
Copy link
Member

@Kleisli @michael-binder Sorry guys - no. The overrides required by this pull request are still too extensive to include and maintain, and the core still does not allow the right integrations to work with this particular translation action.

The best best for now seems to be either translating via the IRRE relation that's in the parent element, or using an assistant extension like l10n_manager which I am told has specific support for Flux elements.

@glucka
Copy link
Member

glucka commented Apr 23, 2018

PR #1545 sould solve this too.

@NamelessCoder
Copy link
Member

Solved as part of #1563 - thanks for your work all the same!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants