Closed (fixed)
Project:
Translation Management Tool
Version:
8.x-1.x-dev
Component:
Core
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
30 Mar 2015 at 13:38 UTC
Updated:
2 May 2015 at 16:44 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
Anushka-mp commentedComment #2
Anushka-mp commentedComment #3
sasanikolic commentedParagraphs work now.
Possibly have to fix those empty divisions under the title.
Comment #5
sasanikolic commentedForgot a debug in the patch, will remove it when I get a review and when we have a solution for the test fail.
Comment #6
berdirWrong use.
That method name doesn't make sense for me. It doesn't get levels (I'm not sure what that means ;))
What it does is extract translatable data, so I'd go with extractTranslatableData(), just like in the config source.
Also, documentation needs work, no need to need to mention paragraphs here, instead it should be documented in the code below what we're doing and why.
The whole block can be removed again.
The reason you get empty divs is because you define labels for elements that then don't have any data inside them. You need to make sure that you only set this when you actually have data, by setting it inside the if(), for example.
Also, the next step is to add a setting to tmgmt_content that allows to control to which entity types this should be done. There we can also describe this behavior.
Needs tests, which will obviously only work after core and paragraphs is patched. But we can write them already.
Comment #7
sasanikolic commentedDone the embedded references and a test for the checked reference fields.
Comment #8
berdirCool, this is starting to come together, still quite a bit of work needed...
the new file should have an empty definition for the new key, embedded_fields: { }
This part here is interesting. We should do the same for embedded references. And if they don't have the source language, then probably skip them.
I can see that it is a function :)
Always start with a verb in third person when describing what a function does. In this case...
"Extracts translatable data from an entity."
Wondering if we should rename this to $entity.
Also, description missing, and @return as well.
Now, we need to use the new setting here, because right now, we don't use it anywhere.
Instead of the $exclude_field_types here, what we want to check is that the field name is in the list of enabled fields for that entity type.
Also note that we only consider untranslatable fields here. (I will get back to this).
Then, we need tests for the actual logic here.
I would suggest a new method in ContentEntityUnitTest. You can start with one of the existing methods, for example for EntityTest. And then add a reference field (this needs entity_reference.module) that points to entities of the same type (for simplicity). Enable that field in your settings, then create two entities: A and B that references A. Then request data for it, and save it back.
Saving back will then fail, because it is not implemented yet :)
You should add a few asserts here about fields that should *not* be present, because we filter them out.
Coming back to the isTranslatable() check from above. We want to check that here as well. Which means that most fields will go away and we will need to explicitly set one up for testing. Just like in the unit test.
The description above should then also explain that this will only work for fields that are not translatable.
@param for form callbacks are not needed, just remove it. However, the description should somehow reference about which form it is.
check coding standard here.
Comment #9
sasanikolic commentedPart of the third patch. Needs to be finished.
Comment #10
berdirOk, lots of additional checks, improved tests, implemented saving translations. Could use some code reviews and testing, I've only run the tests and the code is pretty complicated in some places, a bit of refactoring wouldn't hurt.
Comment #11
miro_dietikerYay, this looks awesome. Some notes.
Preferrably add a comment about the embedded fields recursion of the call and the intermediate data structure.
Hm, altered indentation looks wrong to me.
Why now processing computed values?
Comment #12
berdir2. Keep in mind that it is inside an anonymous function, it's phpstorm that changed that.
3. because getProperties() doesn't return them in the first place, that was a left-over of when it did do that. Not directly related to the other changes.
Comment #13
berdirWasn't aware that entity_reference_revisions has a separate data type (which is called entity_revision_reference because reasons), this should work with both now.
Comment #14
sasanikolic commentedSaving was broken. This should fix it.
Comment #16
miro_dietikerCorrected...
Comment #18
miro_dietikerCommitted. More fun in followups.