Based on #1498634: [meta] Define revision/translation SQL schema for core entities (no patch) there should be a new schema. The name should be users_translation it's connected to users directly.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Issue tags: +D8MI, +language-content

Add tags

Soul88’s picture

Status: Active » Needs review

Hi, everyone,

While thinking about the possible schema for users translatable tables I ran into a question: what are the possible use-cases when some user fields should be translated?

It seems to me to be a very specific demand when users need to have different username or password according to the language.

So the question is: do we really need to have 2 more translatable tables for users?

andypost’s picture

Status: Needs review » Active

Another trouble with user schema that there's a 2 langcode fields - langcode and preferred_langcode

Suppose only signature could be translatable and require "mirror-table"

Gábor Hojtsy’s picture

Title: Define user translation schema » Refactor user entity properties to multilingual
Status: Active » Postponed
Gábor Hojtsy’s picture

Adding mroe D8MI tags. This is still postponed now on #1818570: Convert users to the new Entity Field API.

Gábor Hojtsy’s picture

Status: Postponed » Active

I think all dependencies have landed.

plach’s picture

I thought we weren't allowed to address these issues after feature freeze unless we switched-in a new storage controller.

Anyway, related issue #2068329: Convert user SQL queries to the Entity Query API.

Gábor Hojtsy’s picture

Yeah so long as there are no direct SQL queries, then changing the schema is not an API change.

plach’s picture

Issue summary: View changes
FileSize
5.21 KB

This is a first attempt, it won't probably apply as I rolled this from the #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions branch but it should be a good starting point.

plach’s picture

Status: Active » Needs work
plach’s picture

+++ b/core/lib/Drupal/Core/Session/SessionHandler.php
@@ -87,14 +87,14 @@ public function read($sid) {
+      $values = $this->connection->query("SELECT u.*, s.* FROM {users_field_data} u INNER JOIN {sessions} s ON u.uid = s.uid WHERE u.default_langcode = 1 AND s.ssid = :ssid", array(
...
+          $values = $this->connection->query("SELECT u.*, s.* FROM {users_field_data} u INNER JOIN {sessions} s ON u.uid = s.uid WHERE u.default_langcode = 1 AND s.sid = :sid AND s.uid = 0", array(

@@ -102,7 +102,7 @@ public function read($sid) {
+      $values = $this->connection->query("SELECT u.*, s.* FROM {users_field_data} u INNER JOIN {sessions} s ON u.uid = s.uid WHERE u.default_langcode = 1 AND s.sid = :sid", array(

We should refactor these to use an entity query + a SQL query for the session data. It's unfortunate, but we cannot assume user enetities will live in a SQL database.

catch’s picture

I think we should look at taking that out of the session reading completely. Some of the session refactoring issues are also dealing with this.

Most of the time you only care about:

1. User is anonymous or authenticated
2. If authenticated, what the user ID is.

Beyond that, can always get the full user by loading.

plach’s picture

Issue tags: +dead-line

Makes sense

plach’s picture

Issue tags: +sprint

And tagging for sprint to get more eyes on this.

plach’s picture

Status: Needs work » Needs review
FileSize
4.93 KB

Rerolled

Status: Needs review » Needs work

The last submitted patch, 15: user-ml_schema-1498664-15.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
4.1 KB
8.42 KB

Fixed session writing.

plach’s picture

FileSize
5.25 KB
13.68 KB

And this should fix views integration.

The last submitted patch, 17: user-ml_schema-1498664-17.patch, failed testing.

plach’s picture

FileSize
843 bytes
14.5 KB

Last fix for tonight

The last submitted patch, 18: user-ml_schema-1498664-18.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 20: user-ml_schema-1498664-20.patch, failed testing.

andypost’s picture

+++ b/core/modules/dblog/src/Controller/DbLogController.php
@@ -223,7 +223,7 @@ public function overview() {
-    if ($dblog = $this->database->query('SELECT w.*, u.name, u.uid FROM {watchdog} w INNER JOIN {users} u ON w.uid = u.uid WHERE w.wid = :id', array(':id' => $event_id))->fetchObject()) {
+    if ($dblog = $this->database->query('SELECT w.* FROM {watchdog} w WHERE w.wid = :id', array(':id' => $event_id))->fetchObject()) {

is wrong, should join on user field table instead

plach’s picture

@andypost:

Actually it seems the join is no longer needed as a user load is performed just afterwards (I see the room for some optimization here ;)

Any chance you could bring this a bit forward? I will be pretty busy with #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions these days.

andypost’s picture

Status: Needs work » Needs review
FileSize
5.06 KB
18.61 KB

Fix some views tables and reverted #20, because uid & name used formatMessage() latter
@plach Yes, this could be improved but that's out of scope

Status: Needs review » Needs work

The last submitted patch, 25: 8.x-et-user-ml_schema-1498664-andypost-25.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
9.25 KB
27.86 KB

more fixes

andypost’s picture

The last submitted patch, 27: 8.x-et-user-ml_schema-1498664-andypost-27.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 28: 8.x-et-user-ml_schema-1498664-andypost-28.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
4.22 KB
41.41 KB

Status: Needs review » Needs work

The last submitted patch, 31: 8.x-et-user-ml_schema-1498664-andypost-31.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
4.25 KB
44.46 KB

Status: Needs review » Needs work

The last submitted patch, 33: 8.x-et-user-ml_schema-1498664-andypost-33.patch, failed testing.

andypost’s picture

@plach I've no idea how to fix the left 2 tests, please take a look
1) PathLanguageTest - fails randomly
2) HandlerFieldUserNameTest - somehow makes wrong relation

plach’s picture

Ok, I will have a look as soon as I can. Thanks!

andypost’s picture

Status: Needs work » Needs review
FileSize
1.93 KB
46.39 KB

Only PathLanguageTest left

Status: Needs review » Needs work

The last submitted patch, 37: 8.x-et-user-ml_schema-1498664-andypost-37.patch, failed testing.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
46.4 KB
972 bytes
penyaskito’s picture

Run PathLanguageTest several times locally, and always got green.

Status: Needs review » Needs work

The last submitted patch, 39: 1498664-user-properties-multilingual-39.patch, failed testing.

Gábor Hojtsy’s picture

Probably also needs to take account #2074255: Add changed time tracking to users now?

plach’s picture

AAMOF in the latest patch PathLanguage is not failing :)

andypost’s picture

Status: Needs work » Needs review
FileSize
2.73 KB
48.94 KB
jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

This all looks good. It's also blocking several other issues. And the tests are finally passing. So... let's get it in!

jhodgdon’s picture

This will conflict with #2319671: ViewsDataController: Step1: Move entity views data into controllers... not sure which should go first?

plach’s picture

Priority: Normal » Major
Issue tags: -dead-line +beta-deadline
plach’s picture

Issue tags: -beta-deadline +beta deadline
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Controller/ExceptionController.php
    @@ -408,8 +408,9 @@ protected function decodeException(FlattenException $exception) {
    -      $backtrace = $exception->getPrevious()->getTrace();
    -      $backtrace[0]['line'] = $exception->getLine();
    +      $previous_exception = $exception->getPrevious() ?: $exception;
    +      $backtrace = $previous_exception->getTrace();
    +      $backtrace[0]['line'] = $previous_exception->getLine();
    

    How come and this does not look quite right. Since $backtrace[0]['line'] was being set to the wrapper exception's line. Now it is not. Perhaps we should just check that we have a previous exception in the if above.

  2. +++ b/core/modules/system/src/SystemManager.php
    @@ -146,7 +146,7 @@ public function listRequirements() {
       public function fixAnonymousUid() {
    -    $this->database->update('users')
    +    $this->database->update('users_field_data')
           ->expression('uid', 'uid - uid')
           ->condition('name', '')
           ->condition('pass', '')
    

    The {users} and {users_field_data} table do not use a serial field anymore therefore I think we can remove this. As it stands this change is incorrect because it this did occur then we'd have a mismatch between the two tables.

  3. We need to fix {users}.pass field in core/scripts/password-hash.sh since that field is part of the users_field_data table.

I've checked the indexes created on the {users} and {users_field_data} tables are they are comparable with HEAD. Nice.

andypost’s picture

Status: Needs work » Needs review
FileSize
3.06 KB
49.41 KB

1) reverted
2) removed the function and usage because #204411: Anonymous user id breaks on MySQL export/import (xID of Zero get's auto-incremented on external xSQL events) was for d6 and does not reproduce since
3) fixed

plach’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

We need a change record for this - like the other multilingual conversions.

andypost’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
49.41 KB

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 53: 8.x-et-user-ml_schema-1498664-andypost-53.patch, failed testing.

alexpott’s picture

Status: Needs work » Reviewed & tested by the community

#54 was a testbot issue.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed b3695e8 and pushed to 8.0.x. Thanks!

  • alexpott committed b3695e8 on 8.0.x
    Issue #1498664 by andypost, plach, penyaskito | dawehner: Refactor user...
plach’s picture

Whoo-hoo! @andypost+++++++

Gábor Hojtsy’s picture

Issue tags: -sprint

Thanks folks! Great job!

Status: Fixed » Closed (fixed)

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