Problem/Motivation

Views' DisplayPluginBase::isBaseTableTranslatable() examines all entity types defined and if the first translatable one matches the base table of the view, only then would it return TRUE. With multiple entity types translatable (such as enabling block content and node modules), this method will not return correct information.

Proposed resolution

DisplayPluginBase::isBaseTableTranslatable() should check the entity type that the base table of the view belongs to instead.

Remaining tasks

Tests.

User interface changes

None.

API changes

None.

CommentFileSizeAuthor
#21 2450205-21.patch5.28 KBplach
#18 2450205-18.patch5.43 KBmrjmd
#17 interdiff.txt746 bytesmrjmd
#17 2450205-17.patch8.72 KBmrjmd
#11 interdiff.txt1.55 KBGábor Hojtsy
#11 2450205-10.patch4.7 KBGábor Hojtsy
#9 interdiff.txt3.75 KBGábor Hojtsy
#9 2450205-9-TEST-ONLY.patch3.26 KBGábor Hojtsy
#9 2450205-9.patch4.71 KBGábor Hojtsy
#5 interdiff.txt1.12 KBGábor Hojtsy
#5 2450205-5.patch1.27 KBGábor Hojtsy
#2 2450205-2.patch1.62 KBGábor Hojtsy
interdiff.txt1.62 KBdawehner
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
Issue tags: +language-content, +sprint
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Active » Needs review
FileSize
2450205-2.patch1.62 KB

The fix seems to make sense to me. Let's send this for testing for a start.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Plach made a good point, we actually don't have to test the tables but just rely on $entity_type->isTranslatable()

plach’s picture

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

Yep, checking entity type translatability should be enough, given that's what will trigger the creation of data tables.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Issue summary: View changes
FileSize
2450205-5.patch1.27 KB
interdiff.txt1.12 KB

Doing that then :)

Gábor Hojtsy’s picture

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

Was looking into providing test coverage, but could not reproduce the bug and not sure why. I installed Standard, enabled Language module and added another language. I cannot see the language option in the file view but can see in node and user views. I also created a view of custom blocks, can see the language option there too.

I can see how the code does not make sense, but cannot reproduce a scenario where it would fail... Also, the isTranslatable() is a property of the entity type (does not require content translation module to set any translatability). Not sure if we want to tie this to ACTUAL translatability (ie. configured to be translatable) or theoretical translatability (can be configured to be translatable). It is currently the later. If its not currently configured to be translatable, then it should not have translations normally.

@dawehner or @plach: Can you help me with steps to reproduce?

Gábor Hojtsy’s picture

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

So I could not reproduce because I was using beta7. Using HEAD I can reproduce. Found that this is due to http://cgit.drupalcode.org/drupal/commit/core/modules/views/src/Plugin/v... via #2429447: Use data table as views base table, if available..

Working on tests.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs review » Needs work

Patch looks good, setting to NW for tests...

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Status: Needs work » Needs review
FileSize
2450205-9.patch4.71 KB
2450205-9-TEST-ONLY.patch3.26 KB
interdiff.txt3.75 KB

1. Made the popup handler use the same condition (multilingual & translatable) that the display overview used.
2. Fixed the information message that shows for the case when the feature is not supported. It depends on the view itself not if generally the user has translatable entity types or not.
3. Added tests for various stages of enabling multilingual and how that changes (or not) whether the view supports it.

Note that the test-only patch has the textual fix so it does not fail on that :)

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Issue tags: -Needs tests
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
FileSize
2450205-10.patch4.7 KB
interdiff.txt1.55 KB

assertTranslationSetting() => checkTranslationSetting() to make it easier to debug. Suggestion from @dawehner on IRC.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

Thank you!

The last submitted patch, 9: 2450205-9.patch, failed testing.

The last submitted patch, 9: 2450205-9-TEST-ONLY.patch, failed testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: 2450205-10.patch, failed testing.

mrjmd’s picture

mrjmd CreditAttribution: mrjmd commented
Assigned: Unassigned » mrjmd

I'm going to have a look at this right now.

mrjmd’s picture

mrjmd CreditAttribution: mrjmd commented
Assigned: mrjmd » Unassigned
Status: Needs work » Needs review
FileSize
2450205-17.patch8.72 KB
interdiff.txt746 bytes

This should do it.

mrjmd’s picture

mrjmd CreditAttribution: mrjmd commented
FileSize
2450205-18.patch5.43 KB

Oops, that interdiff was correct but the actual patch wasn't :/. One more try.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Status: Needs review » Reviewed & tested by the community

Trivial fix compared to #12, so should be back to RTBC. Thanks!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +needs rero 
git ac https://www.drupal.org/files/issues/2450205-18.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  5562  100  5562    0     0  18556      0 --:--:-- --:--:-- --:--:-- 20152
error: patch failed: core/modules/views/src/Plugin/views/display/DisplayPluginBase.php:2610
error: core/modules/views/src/Plugin/views/display/DisplayPluginBase.php: patch does not apply
plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -needs rero +Needs reroll
FileSize
2450205-21.patch5.28 KB

Rerolled

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Issue tags: -Needs reroll
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed a805ce2 and pushed to 8.0.x. Thanks!

  • alexpott committed a805ce2 on 8.0.x 
    Issue #2450205 by Gábor Hojtsy, mrjmd, plach: Translation settings don't...
Gábor Hojtsy’s picture

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

Yay, thanks all!

Status: Fixed » Closed (fixed)

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