Closed (fixed)
Project:
Translation Management Tool
Version:
7.x-1.x-dev
Component:
Core
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Reporter:
Created:
22 Mar 2012 at 23:40 UTC
Updated:
15 Jun 2013 at 13:40 UTC
Jump to comment: Most recent file
Comments
Comment #1
miro_dietikerNote that the cascading might be recursive and might end up in adding everything.
Node references can point to children and other node references can point to parents.
This isn't that trivial and there are different types of references that are possible.
(e.g. Tags / Categories, Menu link items, Node references, entity references, ...)
Comment #2
miro_dietikerRelated #1973510: Support Field Collection field types
The current idea is to add a step in the checkout to ask, if related items should also be added.
I suggest that source plugins implement a hook (or a method per item) where they can return suggested referenced items.
I guess, we need a hook anyway as e.g. the node or entity source plugin cannot cover this on its own, but the different fields that implement relationships (entity reference, media reference, book hierarchy...) need to be supported.
Also i guess, a plugin should be able to implement the hook and provide multiple levels of suggestions:
* must (configured to be added without confirmation)
* recommended (e.g. attached media)
* suggested (e.g. siblings)
I would expect that e.g. fields / field instances are extended with configuration fields to define, if they are must / recommended / suggested.
Also, whenever i add some source item, it should be resolved too and its suggestions added.
Recursions need to be prevented ;-) (things already in / or in suggestions, terminated)
Without suggestions from the items, this step is skipped.
Comment #3
miro_dietikerThis was just requested by a client. We currently negotiate if we can push this to production soon :-)
Please contact us if you want to support development of this feature too.
Comment #4
berdirYes, that's along the lines of what I was thinking for this as well.
I'd expect the hook to look like this:
When we have job items, then we can easily fetch the data, calculate the word count of them and things like that.
We can also additionally call a method on the source plugin if we want to.
About prevention recursions, TMGMTJob should offer a hasJobItem($job_item), so hook implementations can avoid returning them and we can make an additional check before we display/add it or so.
This could result in quite a few $job->getItems() calls, which does an EFQ query each time. We should probably store that on the entity to prevent that (addItem() would have to clear/update that of course).
Not yet sure about the UI but I don't want a wizard thing with "steps" I think.. I'd prefer having a link/button somewhere below the items or so. That could possibly already display how many suggestions exist but we need to be careful with the performance here, the checkout page is already slow. We can load it with ajax if we want to. As an initial version, a checkboxes element that displays each job item with "@title (@word_count words)" should allow to test and demonstrate this.
Not sure about the whole priority/levels stuff. Let's leave that out for an initial implementation.
As a first implementation, let's implement it for file/image fields as that's what we need. This should probably live in tmgmt_entity.module for now. Make sure it's fast if files aren't configured to support translations (there's a function that returns translatable entity types in that module), don't check anything in that case. Next, loop over the job items, get the field instances and look for file/image fields. If we find any, check the translatable setting, only do something if it's untranslatable (as translatable in this case likely means a different image/file per language, not a translation). If we get that far, load the references image/file, get the translation handler and check if there already is a translation for the target language that is not outdated. If that also is not the case, create a job item and return it.
Comment #5
berdirNote: Once we have the hook implementation for files/images, it should be easy to extend it to support entityreference.module and references.module as well in the same.
Comment #6
berdirRe-titling
Comment #7
LukyLuke_ch commentedFirst prove of concept and not fully working patch...
Comment #9
berdirA hook implementation doesn't need @param/@return documentation. Instead, hook_tmgmt_source_suggestions() should be added to tmgmt.api.php as documentation with simple example.
Comments should be wrapped at 80 characters.
The file field name should automatically be part of the job item title, which we should also display. Instead, the title should specify why it's referenced. Something like Referenced file from field @field_label ($instance['label'])
Theming shouldn't be based on id's, especially id's that are part of forms, which often get --2 and similar suffixes when ajax is involved.
Should use #type submit I think.
Also needs a #limit_validation_errors => array() so that e.g. a required comment field of a translator submit form doesn't result in an validation error and prevents that the submit callback is executed.
As the test exceptions say, a tableselect needs a #header. Why do you define this outside of the function? If you need a placeholder, you should probably have a #type => 'container' around it that has the necessary id/class.
Changing the target language/translator also results in ajax requests, that's not enough. Probably set a a $form_state['show_suggestions'] or similar flag in the submit callback of your button.
First line of a function should always be in third tense. SetS ..., ReturnS ...
It's not necessary to document $form/$form_state and @return for form builder/callbacks.
lowercase hook and without the argument. Also > 80 characters :)
Probably makes sense to cache it but then we should remember to clear in case we add suggested job items, as these could have additional suggestions.
Either directly (by removing it in the submit callback that will add the selected suggestions), or indirectly (by building some sort of cache key based on the current job items). The second is a bit more complicated but could also handle different ways of a job getting new job items. There are ideas about a shopping cart like mode.
As mentioned above, this should have three columns, the job item title/source title, a reason and the word count. Which we have to calculate as discussed.
Comment #10
miro_dietikerDon't forget sources/entity/tmgmt_entity.api.php and document the new hook.
So you're creating items as suggestions to allow calling getWordCount(). I wanted to suggest postponing the item creation till they're selected, but i see the need for this to have the getWorkCount available... hmm :-)
You're creating a title in the suggestion hook. If not provided, you switch back to the entity identifier (Note it's Item #id, not Job #id). I'm not so happy with this mixup.
I would expect a suggestion showing the entity label() with possibly a (different formatted) reason (the original item that suggested it.). Also nor label nor identifier are unique. We need e.g. a column adding entity type and even bundle here.
suggestions
Keep in mind that once you add a suggestion, it needs to retrigger the hook. Thus the signature should also allow incremental calls for a job_item.
"available."
Comment #11
miro_dietikerHaha, nice... Cross review ;-)
Comment #12
berdirYes, creating a job item is as defined by me above, it gives us the possibly to show the word count (once we slightly refactor it, that is) and ignore empty ones early on. It's not a costly operation as we don't save them.
the hook needs to be documented in tmgmt.api.php, not tmgmt_entity.api.inc, the latter is just the first implementation.
Other than that, we're pretty much consistent :)
Comment #13
LukyLuke_ch commentedThanks for the review, have implemented the above suggestions.
Also refactored 'TMGMTJobItemController' to calculate translatable words after create the job item and not only before save. This could be done prettier, but because there is no getter and setter for TMGMTJobItem::data, I did it this way.
Need a hint now about what to do now with this suggestions now. Is a link needed between the original job and the suggestion, or is the new one just saved and forgoten after in this list?
Second thing: Is there already a method implemented to get all translations (and jobs) from an entity? Because those should be excluded from the suggestions...
Comment #15
LukyLuke_ch commentedSave and check for duplicates, already translated and items from other jobs implemented.
This now should represent a working copy with currently only direct attached nodes (no recursion).
Tests still outstanding.
Comment #17
LukyLuke_ch commentedCorrected tests and started with a test for the suggestions.
Comment #19
berdircount is already a verb, no need for "do", just countWords() is enough. It's save/create, not doSave/doCreate :)
Careful with changing unrelated code. See http://webchick.net/please-stop-eating-baby-kittens.
no space before t(.
This is node specific code, you should use entity_label(), entity_uri() (returns a $uri array, you want $uri['path']) and $bundle, which you already have extracted above.
Note that node and entity is not the same thing. All kinds of things can be entities, a node is a one example entity type and a node type is an entity bundle (a variant of an entity type with different fields/configuration). A file is also an entity type but a file is not a node. And jobs and job items are entities too but not nodes and they're also not fieldable/translatable (*that* would be crazy ;))
That said not sure if we need this at all, see below.
Second sentence is not correct. Translatable fields have a reference too but we can't just translate it because we have to assume that a translatable field will get a completely different reference (e.g. a different file instead of the same file with translated alt/title attributes).
.. check for a supported field type.
supported because just because we don't implement it here doesn't mean it's invalid and field type because we also have entity types here and need to be careful to not mess things up.
The last sentence should be within the if to make clear that it only applies if the if is true :)
Ah didn't at first, I think this might be causing some of the nested-recursion @todo/names below. this is a tmgmt_job_item. That's not the same thing as a tmgmt_job. We have a single job, with a list of job_items. job_items can't contain other jobs or job items, just data. New job items will be added to the job, so as long as you add the suggestions to the job and force a re-calculation of the suggestions, it will just work for the new ones as well. To test, it you could add a file field to your files ;)
referenced now just says from which job item this comes from. We could extract that information automatically for all suggestions if we return the originating job_item as 'from_job_item' => $item. Because that referenced IMHO should say is *why* it is suggested. (Referenced file/image of field @field_label).
tmgmt_i18n_string has a dependency on i18n_string, so this shouldn't be necessary.
No, that's not what we want here :)
Nor this, that was just necessary because of the wrong code in doCountWords(). Should be possible to revert all of this.
This should be moved to a separate test in a separate file as we will need separate dependencies for this to work (file_entity) as it's not possible to add fields to files without that module.
hook instead of tmgmt_entity.
Same for the changes in here.
$form_state['values'], not 'input', input contains the original, unvalidated post values and should not be used directly.
Also don't think it's necessary to do all this checks. This is our form that we have defined. Most of that should always exist, at least the $form_state['tmgmt_job'] and we can imho safely assume that it's an instance of the object we except if it exists.
This can only go wrong if someone messes with our form and then they're simply doing it wrong and showing an error instead of not doing anything silently is actually better imho :)
Also, seems like the tmgmt_job key below tmgmt_suggestions should actually be tmgmt_job_item
& is not necessary for an object.
Hm, addItem() (should maybe be called createItem()) is more for creating new job items, we already have one, so it might be easier to just do $item->tjid = $job->tjid; $item->save();
Might be easier to not make this an ajax action but a normal form submit. Because this will likely influence the whole job, word count will change, translator quotes that could already be displayed will change and more. What it probably should do is clear $form_state['tmgmt_suggestions'] to force a re-calculation of the suggestions.
I guess that TRUE is a test left-over?
We don't need to recursively call something as we don't have a nested structure, we have a job with a list of job items. I'm actually not yet sure if there are cases that could lead to a recursion, we'll see.
This can be moved into the if I think, as there is only a need to clean the list if it's re-calculated?
As written above, $entity should is actually a $job_item or $item.
There should always be an $entity->label(), so you can make that argument requirement and rely on it. Same for $referenced. You already initialize it to NULL and always pass it to this function, so it doesn't need a default value. And if the default should be '', then use that where you define the default value above. Also not sure why referenced is a #type markup and an array and title isn't? That should result in the same result?
While I'm at it, I don't really see the use of this helper function as it is now. It seems to result in more code, not less. If it actually should have a use then I'd just pass $result in and let it handle the rest with default values and stuff. Or alternatively inline it, as you're not going to use it from multiple locations.
Comment #20
miro_dietikerWe have discussed this here and we have extended the hook for the suggestions to:
Thus, we allow a later implementation in core to incrementally fire the hook for newly added items only.
For now, core will always provide $items with all items from the $job.
Comment #21
LukyLuke_ch commentedThat's true, but I decided to not change the whole API because of that. Added a new funciton addJobItem(TMGMTJobItem $item) to the TMGMTJob class for doing this.
There are no values in $form_state['values'] but in $form_state['input'], so I have to use them. But cleaning $id now before I use it as a key.
You mean DuckTyping? :)
I rather check things and "be sure" with what data I work than to just assume that they are correct - also if I am the only one who uses the functions and data structures ;)
Even we must do this here inside the hook_tmgmt_source_suggestions, because as I understand it may be possible to add here also referenced entities which links to ther entities which have also links to others...
For example when you have an entity with a node-reference which also has a node reference with some images or files. Then all these should be suggested, no?
As far as I know, drupal is a multiuser system and therefore it is possible that a second user made a translation of a suggestion. Because of that I clean this list on each call ;)
No, it is not really necessarry normally, but it's consequent ;)
That way you see based on the method definition (especially by non-typed attributes) if the attribute is a "pointer" or not or when you assign an object to a variable.
Based on my experiences (especially when you have to deal with 1000s of objects and real big call stacks), you can not always be 100% sure that without the "&" a class is not cloned and merged back after - which effects in much more memory usage. I was run in to heavy problems because of that and solved it by just definig object attributes as a "pointer" and also inline assignements where possible.
But beside that, here it was needed becasue I have overriden $item and not just adjusted some attributes (addItem has given back a new item which I need in the array).
Here an example:
$a and $b are the same, but if you:
now $a is still what it was (stdClass->test=11) and not 33 like $b...
If you pass it as a referece/pointer:
now $a is also 33...
BTW: The same rules apply to method parameters...
Patch follows in next submit.
Comment #22
LukyLuke_ch commentedNext try :)
still without tests for suggestions.
Comment #24
LukyLuke_ch commentedThe Interdiff as requested...
Comment #25
LukyLuke_ch commentedSome Refactoring, code cleanup and test-implementation.
Comment #27
berdirWondering if this should be recalculateStatistics() as it's not just about words.
I'd rather name this addExistingItem(). Doesn't make sense to have addJobItem and addItem().
Document what keys are returned here in each suggestion (. Each suggestion is an array with the keys job_item, ..., ....
No &'s on the job_item.
No idea what you were doing, but using & is officially slower (see http://stackoverflow.com/questions/178328/in-php-5-0-is-passing-by-refer...) because PHP does copy-on-write.
Also, I don't think the if is necessary here, setting it again is practically free and we no longer calculate on create so the only way it could already be created is if it was called explicitly before.
$job is not a job, it's a $suggestion.
If you then enforce here that all documented properties also exist, then you don't have to isset() and stuff wherever you use it.
Not sure if this shouldn't be part of getSuggestions() ? Why would I ever want a non-cleaned list of suggestions?
Same, no &
That check means you will keep a suggestion that doesn't have a job item at all. Either remove or change it so that they are removed in that case as well.
As above, rename and docblock should be changed to "Recalculate statistics and word count."
.. we don't suggest here. ... from the *referenced* entity.
Still missing field label :)
Referenced @type of field @label, @label => $instance['label']
As discussed, let's move this to sources/entity/tmgmt_entity.suggestions.test
getInfo() is usually the first method in a test class and doesn't need a docblock according to the (somewhat weird) current coding standards.
The assertion message (third argument), shouldn't use t(). I know we still have a quite a few of them, the coding standard was changed, so new code shouldn't use it.
Also, let's add some more test here, we should confirm that the job_item has the correct plugin/item_type/item_id values and getWordCount() should return the expected value, to make sure that it was called.
I'd just say Return a list of suggested sources for job items.
I would either use "job items" when you talk about it or "TMGMTJobItem objects" when you want reference the class name. TMGMTJobItems is neither english nor can it be referenced by api.module.
That's not necessary I think, the description above should be enough.
Inconsistent/outdated information, remove reference_link/entity_uri
If a job is no longer submittable, then don't display the suggestions stuff. Once submitted, the job item list is fixed, so showing suggestions is pointless. At least for now, we could consider creating new jobs from this interface later on.
Remove this and make it a non-ajax action, then you don't need the @todo below and the page will simply rebuild and display the updated page. anything could change (default job label, word count, translator quotes, ...). It's pointless to make this an ajax action.
Not necessary, was a misunderstanding I think and you use values now. check_plain() is about protecting against XSS and escaping HTML. That's not relevant here and should only be done before something is printed.
I know we have a different style but I still consider this to be counter-productive.
If really something would not match here, then I want to know. Instead, this will simply silently do nothing and I need to start debugging. The only way I can imagine an error showing up here is during development. And on production, errors are disabled and logged to watchdog.
That's just not really how Drupal code is written and prevents me from scanning it quickly.
You are not changing $entity here.
Comment #28
LukyLuke_ch commentedPatch with latest discussions, more entity tests and gui tests too.
Comment #29
miro_dietikerOk, let me list the suggestions we should support... Possibly create followup issues:
- entity reference (forward / reverse?)
- book hierarchy
- relation
- term reference
- field collections
- extracted internal links in content?!
- (obsolete node reference)
Possibly even more?!
Don't forget multilingual/i18n aspects for each..
Comment #30
LukyLuke_ch commentedAdded following feature requests:
#1999796: Related source suggestions for menu translation
#1999798: Related source suggestions for term references
#1999802: Related source suggestions for field collections
#1999812: Related source suggestions for entities in a menu
#1999820: Related source suggestions for content links
#1999838: Related source suggestions for i18n_string
#1999922: Related source suggestions for relation module
#1999926: Related source suggestions for entity_reference
#1999928: Related source suggestions for book module
Comment #31
miro_dietikerThis still needs review...
Comment #32
cgalli commentedApplied and tested the patch.
It works as described for fields attached to a file entity in a content type.
Comment #33
berdirOk, this looks quite nice.
Played around with the UI a bit, made some changes there, also added a test implementation and moved the UI tests to tmgmt_ui so that we can test some additional things like suggestions of suggestions (which was broken ;))
Uploading patch, will explain the changes in a self-review in the next post.
Want to open two UI issues:
- Remove the translation of prefix of the item label, that's stupid and uses too much space :)
- Change the technical Plugin label to a type label that the controller can provide. So node can display the bundle (e.g. Article), entity can display the entity type or maybe entity type and bundle, i18n_string can display the object group and so on.
The translation of prefix is a one line change but it's unrelated and will break some tests, but I already tested it, see screenshot.
Comment #34
miro_dietikerNice work! However not reviewed the code recently.
Comment #35
berdirAs promised, some explanations. Committed.
Also opened #2009024: Remove "Translation of" prefix of job item default label and #2009026: Replace Plugin column in items/suggestion listings with "Type".
Not directly related but entity_translation is a dependency, was only in the UI before which was not correct. Allows me to simplify the test dependencies
This is defined in tmgmt.info, doesn't need to repeated here.
As I moved the UI tests, I renamed this to be specific about entity source suggestions.
Added assertions for reason and from_item.
Renamed referenced to reason, from_job to from_item, that was a misunderstand or so, the job is already known, it's the item that we don't know. That didn't cause any problem because it was never used :)
Renamed the save button to add, same for submit callback and so on.
Some comment fixes, we're adding job items to the job, not jobs.
rebuild_suggestions is only used to display it at all, we can assume that it's already set or you would not see the save button, so we don't need to set it. But, we neeed to unset the existing suggestions so that they are recalculated and suggestions of suggestions are loaded.
Not having any suggestions is a valid case, we don't need to recalculate in that case. And we certainly can't have a negative amount :)
Moved the responsibility of how to generate $option from $result completely into the function, makes more sense to me.
Added plugin column, renamed reference to reason.
Removed the link (which I think is just distracting), instead automatically add the from job item label if we have one. Adding the plugin (which will become the type) also lead me to shortening the default reason down to just the field name.
I left the above assertions as then we also have some test coverage over that, but drupalPostAJAX() actually updates the content variable, so you can do normal assertText()/assertRaw assertions on the response and we can submit the form as displayed below.
Comment #36
cgalli commentedWorks perfectly well for the normal case.
One case is confusing:
1) Ask for translation, submit job to local WITHOUT adding suggested elements.
2) Go back to the job, add suggestions. An additional Job item is created.
3) Go to local translator. Only one job item is in the task.
Where is the second job item? And how can I submit it to translation?
Comment #37
cgalli commentedwrong place to report a bug.
Please refer to #2009764: Suggestions allow a job item to be created after subbmission