Problem/Motivation

  1. Only entities which are fieldable (can have configured fields) are currently configurable as translatable. If they only have base fields which are translatable, that is not enough. This makes it impossible to set shortcuts to be translatable.
  2. Shortcuts do not have a default Edit tab to make the translation tab appear. This was manually added in #2085907: Ensure all configuration entities have an Edit/Configure tab by default to others and may be automated later, see #2095117: Menu system should provide a default tab if none exists but that is not going anywhere, so we need to add manually for now.
  3. When shortcuts are displayed in the toolbar, their original language version is used at all times, not their translations.

Solving these would make shortcuts configurable to be translatable and actually translatable.

Proposed resolution

  1. Fix the code to first try and collect translatable fields (including base fields) and if none exist, then bail with the checkbox, if there was no translatable field. Should not tie to configurable fieldability.
  2. Add a default edit tab.
  3. Load a translated version of the shortcut entity appropriate for the toolbar at the time.
  4. Add tests based on the standard content translation test facility, but...
    1. This test facility needs updating to not create a configurable field if the entity is not fieldable.
    2. However let the test provide the fieldName for a base field so the tests are still fully operational (and remove this from menu content tests, where fieldability is supported).
    3. This uncovered translation permissions not being created because shortcut sets are not marked as bundles of shortcuts (but they are, see Shortcut.php)

Remaining tasks

Review. Commit.

User interface changes

Shortcuts will finally be configurable to be translatable. They will have visible translate tabs that allow translation of shortcuts. Finally shortcuts will appear translated in the toolbar.

API changes

None.

CommentFileSizeAuthor
#36 interdiff.txt1.41 KBGábor Hojtsy
#36 shortcut-entity-translatable-36.patch10.5 KBGábor Hojtsy
#35 interdiff.txt932 bytesGábor Hojtsy
#35 shortcut-entity-translatable-35.patch10.45 KBGábor Hojtsy
#29 shortcut-entity-translatable-29.patch10.53 KBGábor Hojtsy
#27 interdiff.txt1.24 KBGábor Hojtsy
#27 shortcut-entity-translatable-27.patch11.2 KBGábor Hojtsy
#22 interdiff-debug.txt1.07 KBGábor Hojtsy
#22 shortcut-entity-translatable-22.patch11.21 KBGábor Hojtsy
#19 interdiff.txt1.18 KBGábor Hojtsy
#19 shortcut-entity-translatable-19.patch11.11 KBGábor Hojtsy
#17 interdiff.txt1.33 KBGábor Hojtsy
#17 shortcut-entity-translatable-17.patch11.09 KBGábor Hojtsy
#14 interdiff.txt2.68 KBGábor Hojtsy
#14 shortcut-entity-translatable-14.patch10.9 KBGábor Hojtsy
#12 Translations_of_Add_content___Drupal_8_x.png322.48 KBplach
#10 interdiff.txt618 bytesGábor Hojtsy
#10 shortcut-entity-translatable-10.patch9.32 KBGábor Hojtsy
#8 shortcut-entity-translatable-8.patch9.36 KBGábor Hojtsy
#8 interdiff.txt4.53 KBGábor Hojtsy
#7 shortcut-entity-translatable-7.patch4.82 KBGábor Hojtsy
#6 interdiff.txt507 bytesGábor Hojtsy
#6 shortcut-entity-translatable-6.patch4.92 KBGábor Hojtsy
#1 shortcut-entity-translatable.patch5.41 KBGábor Hojtsy
#1 shortcut-entity-translatable-easier-change-review.txt3.12 KBGábor Hojtsy
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
FileSize
shortcut-entity-translatable-easier-change-review.txt3.12 KB
shortcut-entity-translatable.patch5.41 KB

Uploading an easier to review patch which shows the removed condition commented out (and does not have indentation changes for the removed wrapping condition). And an actual patch for testing that does (but looks like a lot more changes which is just for the indentation).

YesCT’s picture

YesCT CreditAttribution: YesCT commented 
+++ b/core/modules/content_translation/content_translation.admin.inc
@@ -89,41 +89,39 @@ function _content_translation_form_language_content_settings_form_alter(array &$
-      // Only show the checkbox to enable translation if the bundles in the
-      // entity might have fields and if there are fields to translate.
-      if ($entity_type->isFieldable()) {
-        $fields = $entity_manager->getFieldDefinitions($entity_type_id, $bundle);
...
+      $fields = $entity_manager->getFieldDefinitions($entity_type_id, $bundle);
+      if ($fields) {
+        foreach ($fields as $field_name => $definition) {

ah, I see. we should not check if it is fieldable... cause base fields are translatable now. ok

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Issue tags: +Needs tests

@YesCT: right, that is the gist of the patch, we try and collect and translatable fields including base fields, and only add the entity bundle level checkbox, if we found any -- instead of inferring that from configurable fieldability which should not be a requirement.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs review » Postponed

In fact, instead of adding that workaround, I'm postponing this on #2310475: Admin theme for translation tabs just works for nodes, which now has complete tests and almost committed.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Postponed » Needs work
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Needs review
FileSize
shortcut-entity-translatable-6.patch4.92 KB
interdiff.txt507 bytes

Now rerolled without the workaround now that #2310475: Admin theme for translation tabs just works for nodes landed.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
FileSize
shortcut-entity-translatable-7.patch4.82 KB

Rerolled since it did not apply due to a dead code line removed.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
FileSize
interdiff.txt4.53 KB
shortcut-entity-translatable-8.patch9.36 KB

Now with a test! Not yet fully passing but resolved a few hair-pulling things:

- ContentTranslationTestBase.php adds a custom field on the entity which obviously cannot be done if its not fieldable, we need to make that conditional, done.
- MenuLinkContentUITest was setting a custom field name, which was useless; it is fieldable so no need to
- Shortcut sets are bundles for shortcuts but this was not defined in the annotation, (automated permissions for translations did not work, so test users could not be created, fun to debug :)

Local fails are now down to the default language titles not properly setting.

Status: Needs review » Needs work

The last submitted patch, 8: shortcut-entity-translatable-8.patch, failed testing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
shortcut-entity-translatable-10.patch9.32 KB
interdiff.txt618 bytes

So the remaining fails are only due to us using the title base field in the test, so we should not try to force the default value either, since the test wants to control it. As soon as we let the test fill in the title value (since we tell it to use it in $this->fieldName), it is fine. So that's it :) Reviews please.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Issue summary: View changes
plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs review » Needs work
FileSize
Translations_of_Add_content___Drupal_8_x.png322.48 KB

The patch looks great, but I tested it and it seems translations are not rendered properly:

Separate issue?

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented

Also, the edit links in the translation overview page do not lead to the correct translation, but always to the one matching the current language.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Issue summary: View changes
Status: Needs work » Needs review
FileSize
shortcut-entity-translatable-14.patch10.9 KB
interdiff.txt2.68 KB

Yeah the shortcuts did not show up translated and I thought to fix it in another issue, but we can just as well prove it here and integrate it into the test. Works :)

On the links in the translation overview, they are borked for ALL entity types, same problem applies to node articles. I'll submit another issue shortly, I think I have a patch.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs review » Reviewed & tested by the community

Great work, RTBC if green

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: shortcut-entity-translatable-14.patch, failed testing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Needs review
FileSize
shortcut-entity-translatable-17.patch11.09 KB
interdiff.txt1.33 KB

Patch was beautifully green locally (multiple times). Making the xpath test more robust, so it should not fail on escaped stuff or whitespace getting in the way.

Status: Needs review » Needs work

The last submitted patch, 17: shortcut-entity-translatable-17.patch, failed testing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Needs review
FileSize
shortcut-entity-translatable-19.patch11.11 KB
interdiff.txt1.18 KB

So the full href may be different if the site is in a directory of course. Use a contains() like other tests do.

Status: Needs review » Needs work

The last submitted patch, 19: shortcut-entity-translatable-19.patch, failed testing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

All right I'm down to having no idea why this breaks. All prior versions of the patch have been green locally. Duh. Will need a patch testing issue splintered with debug messages :(

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Needs review
FileSize
shortcut-entity-translatable-22.patch11.21 KB
interdiff-debug.txt1.07 KB

Decided to use this issue to figure this out. It should not be that far away #famouslastwords

Status: Needs review » Needs work

The last submitted patch, 22: shortcut-entity-translatable-22.patch, failed testing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

Duh, looks like all paths requested display the untranslated shortcut / not language dependent path.

Gábor Hojtsy queued 22: shortcut-entity-translatable-22.patch for re-testing.

The last submitted patch, 22: shortcut-entity-translatable-22.patch, failed testing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Needs review
FileSize
shortcut-entity-translatable-27.patch11.2 KB
interdiff.txt1.24 KB

Looks like path access checking got stricter in the meantime, now we cannot set a shortcut to the admin page, so translations did not save. We should use a less restricted path in the test, the user path is good one.

Status: Needs review » Needs work

The last submitted patch, 27: shortcut-entity-translatable-27.patch, failed testing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Needs review
FileSize
shortcut-entity-translatable-29.patch10.53 KB

Needed a reroll due to #2224761: Add a generic way to add third party configuration on configuration entities and implement for field configuration.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

This new rerolled patch passes on the simpletest UI locally (139 passes, 0 fails, 0 exceptions, 52 debug messages) but fails miserably on run-tests.sh (49 passes 51 fails 34 exceptions 29 messages). Uhm, now what? Neither reproduces the situation happening on testbot.

Status: Needs review » Needs work

The last submitted patch, 29: shortcut-entity-translatable-29.patch, failed testing.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented

I am afraid this won't help, but I am seeing lots of failures on HEAD too when running tests via run-tests.sh (tests pass in the UI). Maybe ask @berdir?

Might this be a OSX issue?

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented 
+++ b/core/modules/shortcut/src/Tests/ShortcutTranslationUITest.php
@@ -0,0 +1,79 @@
+        $path = \Drupal::urlGenerator()->generateFromRoute('<front>', array(), array('language' => $language));
+        $this->drupalGet($path);

From the HTML output:

 <h1>Page not found</h1> The requested page "/checkout" could not be found.

You can't use generated paths with drupalGet(). You end up with checkout/checkout (checkout is the subfolder where drupal is installed).

drupalGet() has an options array, just use that: drupalGet('', array('language' => $language).

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

All right, because it is a page not found, the language will not be recognised either. Duh. Fixing this up then. Thanks @berdir.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Needs review
FileSize
shortcut-entity-translatable-35.patch10.45 KB
interdiff.txt932 bytes

Now uses $language natively on drupalGet(), thanks @berdir for the tip. Still has some debugging code in it which should be removed once/if passes finally. (Passes locally).

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
FileSize
shortcut-entity-translatable-36.patch10.5 KB
interdiff.txt1.41 KB

All right, removing debug code. I think this should be ready to go in now :) @plach what do you think?

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Issue summary: View changes
plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs review » Reviewed & tested by the community

Looks (and works) great, thanks!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

  • webchick committed cc87280 on 8.0.x 
    Issue #2320037 by Gábor Hojtsy: Fixed Non-fieldable entities (with only...
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Issue tags: -sprint

Amazing, so glad this works now :) Not for shortcuts per say, but for the coherence of the system in general.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented

Yep, that's awesome :)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.