https://api.drupal.org/api/drupal/core%21modules%21node%21node.install/f...
The column description says it is used for boolean values. That means we should add 'size' => 'tiny' for this column as it is already done for other boolean columns in the table.

\Drupal\node\NodeGrantDatabaseStorage::write() shows that the value can only be 0 or 1:

            // The record with the original langcode is used as the fallback.
            if ($grant['langcode'] == $node->language()->getId()) {
              $grant['fallback'] = 1;
            }
            else {
              $grant['fallback'] = 0;
            }
CommentFileSizeAuthor
#15 Selection_003.png13.61 KBquietone
#15 Selection_004.png5.97 KBquietone
#14 2613190-14-specify_node_access_fallback_column_size.patch1.3 KBfenstrat
#12 2613190-12-specify_node_access_fallback_column_size.patch1.11 KBfenstrat
#7 node_schema_add_size_for_fallback_field-2613190-7.patch977 bytesLOBsTerr
#3 node_schema_add_size_for_fallback_field-2613190-3.patch495 bytesLOBsTerr
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Chi created an issue. See original summary.

LOBsTerr’s picture

LOBsTerr CreditAttribution: LOBsTerr as a volunteer commented
Assigned: Unassigned » LOBsTerr
LOBsTerr’s picture

LOBsTerr CreditAttribution: LOBsTerr as a volunteer commented
FileSize
node_schema_add_size_for_fallback_field-2613190-3.patch495 bytes
LOBsTerr’s picture

LOBsTerr CreditAttribution: LOBsTerr as a volunteer commented
Assigned: LOBsTerr » Unassigned
Status: Active » Needs review
nicrodgers’s picture

nicrodgers
Primary language English
Location Monmouthshire, UK
CreditAttribution: nicrodgers at Welsh Government commented
Status: Needs review » Needs work

The patch looks ok to me - it makes it consistent with the other boolean fields, so nice spot! However, because it's a change to the schema, it needs a corresponding hook_update_N function to update the size for existing installations.

LOBsTerr’s picture

LOBsTerr CreditAttribution: LOBsTerr as a volunteer commented
Assigned: Unassigned » LOBsTerr
LOBsTerr’s picture

LOBsTerr CreditAttribution: LOBsTerr as a volunteer commented
FileSize
node_schema_add_size_for_fallback_field-2613190-7.patch977 bytes

I have added the update hook. I'm not good in commenting and documenting. Let me know if it is should be updated.

LOBsTerr’s picture

LOBsTerr CreditAttribution: LOBsTerr as a volunteer commented
Status: Needs work » Needs review
LOBsTerr’s picture

LOBsTerr CreditAttribution: LOBsTerr as a volunteer commented
Assigned: LOBsTerr » Unassigned

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

fenstrat’s picture

fenstrat
he/him
Primary language English
Location Australia
CreditAttribution: fenstrat commented
FileSize
2613190-12-specify_node_access_fallback_column_size.patch1.11 KB

Here's a minor modification to #7, inspired by history_update_8101() which uses Database::getConnection()->schema->changeField() rather than db_query().

Status: Needs review » Needs work

The last submitted patch, 12: 2613190-12-specify_node_access_fallback_column_size.patch, failed testing.

fenstrat’s picture

fenstrat
he/him
Primary language English
Location Australia
CreditAttribution: fenstrat commented
Status: Needs work » Needs review
FileSize
2613190-14-specify_node_access_fallback_column_size.patch1.3 KB

#12 was missing a use statement.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs review » Reviewed & tested by the community
FileSize
Selection_004.png5.97 KB
Selection_003.png13.61 KB

@LOBsTerr, thanks for finding this and making a patch. At DrupalSouth 2016, fenstrat did a great job at demonstrating how to review this issue. That resulted in a new patch which I've just tested.

Tested the patch on a freshly installed Drupal 8 and it works.

Here is node_acces before the patch, note that fallback is an int(10).

Database changed
MariaDB [d8]> desc node_access;
+--------------+---------------------+------+-----+---------+-------+
| Field        | Type                | Null | Key | Default | Extra |
+--------------+---------------------+------+-----+---------+-------+
| nid          | int(10) unsigned    | NO   | PRI | 0       |       |
| langcode     | varchar(12)         | NO   | PRI |         |       |
| fallback     | int(10) unsigned    | NO   |     | 1       |       |
| gid          | int(10) unsigned    | NO   | PRI | 0       |       |
| realm        | varchar(255)        | NO   | PRI |         |       |
| grant_view   | tinyint(3) unsigned | NO   |     | 0       |       |
| grant_update | tinyint(3) unsigned | NO   |     | 0       |       |
| grant_delete | tinyint(3) unsigned | NO   |     | 0       |       |
+--------------+---------------------+------+-----+---------+-------+
8 rows in set (0.00 sec)

After applying the patch the status report correctly showed that 'Database updates' was out of date. And then running update.php the update was found and ran successfully.

Here is node_access after the patch and fallback is not a tinyint.

MariaDB [d8]> desc node_access;
+--------------+---------------------+------+-----+---------+-------+
| Field        | Type                | Null | Key | Default | Extra |
+--------------+---------------------+------+-----+---------+-------+
| nid          | int(10) unsigned    | NO   | PRI | 0       |       |
| langcode     | varchar(12)         | NO   | PRI |         |       |
| fallback     | tinyint(3) unsigned | NO   |     | 1       |       |
| gid          | int(10) unsigned    | NO   | PRI | 0       |       |
| realm        | varchar(255)        | NO   | PRI |         |       |
| grant_view   | tinyint(3) unsigned | NO   |     | 0       |       |
| grant_update | tinyint(3) unsigned | NO   |     | 0       |       |
| grant_delete | tinyint(3) unsigned | NO   |     | 0       |       |
+--------------+---------------------+------+-----+---------+-------+
8 rows in set (0.00 sec)

And some relevant screen shots:

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Version: 8.2.x-dev » 8.3.x-dev
Issue summary: View changes
Status: Reviewed & tested by the community » Fixed

Updating IS to show that this is a safe change.

Committed 64c4d8e and pushed to 8.3.x. Thanks!

diff --git a/core/modules/node/node.install b/core/modules/node/node.install
index 6ecfc45..720a7e0 100644
--- a/core/modules/node/node.install
+++ b/core/modules/node/node.install
@@ -224,7 +224,7 @@ function node_update_8003() {
 /**
  * Change {node_access}.fallback from an int to a tinyint as it is a boolean.
  */
-function node_update_8004() {
+function node_update_8300() {
   Database::getConnection()->schema()->changeField('node_access', 'fallback', 'fallback', [
     'description' => 'Boolean indicating whether this record should be used as a fallback if a language condition is not provided.',
     'type' => 'int',

Fixed the update number of commit.

  • alexpott committed 64c4d8e on 8.3.x 
    Issue #2613190 by LOBsTerr, fenstrat, quietone, Chi, nicrodgers: Specify...

Status: Fixed » Closed (fixed)

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