Problem/Motivation

Currently UUID module adds an information about database field for core entities. The uuid and revision uuid fields - are SQL indexes but schema API does not know about this because these fields were not added to indexes property of a schema.

This improvement can help, for instance, in this case: I want to delete all indexes from users table to perform a heavy SQL operation. Programmatically I'm able to obtain all indexes from primary key, unique keys and indexes properties of schema definition of the users table but the uuid index will be missed there.

Proposed resolution

Update logic of uuid_schema_alter() function and populate indexes in schema.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#8 interdiff-2763369-6-8.txt3.19 KBBR0kEN
#8 uuid-schema-api-indexes-2763369-8.patch3.37 KBBR0kEN
#6 interdiff-2763369-2-6.txt3.93 KBBR0kEN
#6 uuid-schema-api-indexes-2763369-6.patch3.34 KBBR0kEN
#2 uuid-schema-api-indexes-2763369-2.patch3.81 KBBR0kEN
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

BR0kEN created an issue. See original summary.

BR0kEN’s picture

BR0kEN
Primary language Ukrainian
CreditAttribution: BR0kEN as a volunteer and at FFW for FFW commented
Status: Active » Needs review
FileSize
uuid-schema-api-indexes-2763369-2.patch3.81 KB
skwashd’s picture

skwashd CreditAttribution: skwashd at Dave Hall Consulting for Dave Hall Consulting commented
Status: Needs review » Needs work

@BR0kEN thanks for the patch. I have committed some fixes for the tests that will conflict with your patch. I found a few issues while reviewing it.

  1. +++ b/uuid.install
@@ -31,12 +31,24 @@ function uuid_schema_field_definition() {
+    foreach (array(
+      'base table' => 'uuid',
+      'revision table' => 'revision uuid',
+    ) as $table_type => $key_name) {

    This is difficult to read. Please define the array before iterating over it. 

  2. +++ b/uuid.install
@@ -31,12 +31,24 @@ function uuid_schema_field_definition() {
+        foreach (array(
+          'fields' => $field_info,
+          'indexes' => array($field_name),
+        ) as $schema_property => $value) {

    Please also assign this array to a variable first. 

  3. +++ b/uuid.test
@@ -67,6 +67,57 @@ class UUIDAPITestCase extends UUIDTestCase {
+   * Checks that schema for tables is correctly defined.

    This is only checking the schemas for core entities. The test description should reflect this. 

  4. +++ b/uuid.test
@@ -67,6 +67,57 @@ class UUIDAPITestCase extends UUIDTestCase {
+      foreach (array(
+        'base table' => 'uuid',
+        'revision table' => 'revision uuid',
+      ) as $table_type => $key_name) {

    Another array to assign to a variable first. 

  5. +++ b/uuid.test
@@ -67,6 +67,57 @@ class UUIDAPITestCase extends UUIDTestCase {
+          // No need to fail here because "revision table"
+          // are not exists for all of the entities.

    How about this?

    // Not all entities have a revisions table. 

  6. +++ b/uuid.test
@@ -67,6 +67,57 @@ class UUIDAPITestCase extends UUIDTestCase {
+        foreach (array(
+          'fields' => array('field', $field_info),
+          'indexes' => array('index', array($field_name)),
+        ) as $schema_property => $data) {

    Please assign array to a variable. 

  7. +++ b/uuid.test
@@ -67,6 +67,57 @@ class UUIDAPITestCase extends UUIDTestCase {
+            $this->assertTrue($schemas[$table_name][$schema_property][$field_name] === $value, "$message is correct.");

    Shouldn't this be assertIdentical()?

BR0kEN’s picture

BR0kEN
Primary language Ukrainian
CreditAttribution: BR0kEN as a volunteer and at FFW for FFW commented
Parent issue: » #2763727: Optimizations and fixes for tests

Why do you think that array, directly passed to foreach, is not readable?

BR0kEN’s picture

BR0kEN
Primary language Ukrainian
CreditAttribution: BR0kEN as a volunteer and at FFW for FFW commented 
+++ b/uuid.test
@@ -67,6 +67,57 @@ class UUIDAPITestCase extends UUIDTestCase {
+          // No need to fail here because "revision table"

I'm pretty sure that we do not need to fail here because, if on this stage something went wrong then problem is deeper than definitions of schemas. Are you have any thoughts/concerns regarding this?

BR0kEN’s picture

BR0kEN
Primary language Ukrainian
CreditAttribution: BR0kEN as a volunteer and at FFW for FFW commented
Status: Needs work » Needs review
FileSize
uuid-schema-api-indexes-2763369-6.patch3.34 KB
interdiff-2763369-2-6.txt3.93 KB
  1. Changed description for testSchemas method of UUIDAPITestCase class.
  2. Replaced assertTrue by assertIdentical in testSchemas method of UUIDAPITestCase class.
skwashd’s picture

skwashd CreditAttribution: skwashd at Dave Hall Consulting for Dave Hall Consulting commented
Status: Needs review » Needs work

As noted the parent issue will result in the need for a reroll. I have have committed #2763727: Optimizations and fixes for tests.

My comment in point 5 was a suggestion for changing the comment to a single line comment.

The arrays really mess with the flow of the code. It really impacts on the ability to quickly look at the code and understand what's happening. The arrays need to be fixed.

BR0kEN’s picture

BR0kEN
Primary language Ukrainian
CreditAttribution: BR0kEN as a volunteer and at FFW for FFW commented
Status: Needs work » Needs review
FileSize
uuid-schema-api-indexes-2763369-8.patch3.37 KB
interdiff-2763369-6-8.txt3.19 KB

Created a reroll with fixes for array definitions.

Status: Needs review » Needs work

The last submitted patch, 8: uuid-schema-api-indexes-2763369-8.patch, failed testing.

BR0kEN’s picture

BR0kEN
Primary language Ukrainian
CreditAttribution: BR0kEN as a volunteer and at FFW for FFW commented
Parent issue: #2763727: Optimizations and fixes for tests » #2763943: Prevent running the tests on environments with PHP lower than 5.4
BR0kEN’s picture

BR0kEN
Primary language Ukrainian
CreditAttribution: BR0kEN as a volunteer and at FFW for FFW commented
Status: Needs work » Needs review
BR0kEN’s picture

BR0kEN
Primary language Ukrainian
CreditAttribution: BR0kEN as a volunteer and at FFW for FFW commented
Parent issue: #2763943: Prevent running the tests on environments with PHP lower than 5.4 »
BR0kEN’s picture

BR0kEN
Primary language Ukrainian
CreditAttribution: BR0kEN as a volunteer and at FFW for FFW commented

@skwashd, let's have this committed!

skwashd’s picture

skwashd CreditAttribution: skwashd at Dave Hall Consulting for Dave Hall Consulting commented
Status: Needs review » Reviewed & tested by the community

Looks good. I'll try to get this committed and a new release out the door this week. I've been busy with other work recently.

  • skwashd committed 5982e91 on 7.x-1.x authored by BR0kEN 
    Issue #2763369 by BR0kEN: Populate information about DB index to Schema...
skwashd’s picture

skwashd CreditAttribution: skwashd at Dave Hall Consulting for Pfizer, Inc. commented
Status: Reviewed & tested by the community » Fixed

Thanks for the patch. I've committed it. I'll be preparing a new release this week.

Status: Fixed » Closed (fixed)

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