Identifiers longer than 63 characters are truncated, causing Views to break on Postgres

Could you add a re-rolled patch for 10.1 and 10.2 ?

#165 applies just fine to 10.1. You can check 10.2 and let us know if it doesn't apply.

Following up #166, I think my guess there was correct.

> It's not immediately obvious what field_test_data_i was originally ... Maybe it was "id"?

EntityReferenceRelationshipTest::setUp() does this:

    // Create reference from entity_test to entity_test_mul.
    $this->createEntityReferenceField('entity_test', 'entity_test', 'field_test_data', 'field_test_data', 'entity_test_mul');

    // Create reference from entity_test_mul to entity_test.
    $this->createEntityReferenceField('entity_test_mul', 'entity_test_mul', 'field_data_test', 'field_data_test', 'entity_test');

Which looks like it's making field_test_data a reference to a entity_test_mul entity, and field_data_test a reference to a entity_test entity.

Both of those entity classes declare:

 *   entity_keys = {
 *     "id" = "id",
 *     "uuid" = "uuid",
 *     "bundle" = "type",
 *     "label" = "name",
 *     "langcode" = "langcode",
 *   },

So id looks like the only property it might be if the name is just a concatenation of field name and property key.

And finally, my interpretation for the truncated name is consistent with other appearances of that comment for the field_data_test field:

3 matches for "Test the forward relationship." in buffer: EntityReferenceRelationshipTest.php
       :
    142:      // Test the forward relationship.
       :      $this->assertEquals(1, $row->entity_test_mul_property_data_entity_test__field_test_data_i);
-------
       :
    222:      // Test the forward relationship.
       :      $this->assertEquals(1, $row->entity_test_entity_test_mul__field_data_test_id);
-------
       :
    281:      // Test the forward relationship.
       :      // $this->assertEquals(1, $row->entity_test_entity_test_mul__field_data_test_id);

So I think the way forward is:

use Drupal\views\Plugin\views\query\Sql;
...
      // Test the forward relationship.
      $alias = Sql::sanitizeAlias('entity_test_mul_property_data_entity_test__field_test_data_id')
      $this->assertEquals(1, $row->{$alias});

So let's give that a whirl (against 10.1.x initially).

Running PHPStan on *all* files.
 ------ ---------------------------------------------------------------
  Line   core/modules/views/src/Plugin/views/query/Sql.php
 ------ ---------------------------------------------------------------
  525    Variable $alias in isset() always exists and is not nullable.

Which is complaining about the pre-existing (not part of this patch) code in Drupal\views\Plugin\views\query\Sql::queueTable():

    // If no alias is specified, give it the default.
    if (!isset($alias)) {
      $alias = $this->tables[$relationship][$table]['alias'] . $this->tables[$relationship][$table]['count'];
...

The "always exists" is certainly accurate, but on first look I'm not convinced by the "is not nullable" part. PHPStan must have a reason for claiming otherwise (and I'd tend to assume it's right and I'm wrong), but the reason is sufficiently non-obvious that I can only question the value of eliminating safeguard code on that basis.

By my reading, if we had two calls to queueTable($table, $relationship = NULL, JoinPluginBase $join = NULL, $alias = NULL) using the same table and relationship arguments, and if the second of those calls had a NULL alias argument, then that null could reach the isset() call. Joining to a database table twice with different aliases and join conditions is 100% a valid thing to do in a SQL query (I wouldn't even call it uncommon), so I see no reason why in principle this couldn't happen.

It does seem like an empty string alias shouldn't be considered valid, though (especially as the alias is used as an array key, and null array keys are coerced to empty strings), so I'm changing that !isset() to empty() so that we can catch that case and hopefully prevent PHP Stan from asking us to delete a pre-existing safeguard.

Re-roll of #179 for 10.2.x and 11.x.

Recommend turning to MR as patches are being phased out.

But appears to have several test failures.

Also issue summary should follow standard issue template.

Thanks!

> Recommend turning to MR as patches are being phased out.

The MR workflow at Drupal continues to not provide immutable patch URLs which is hugely problematic, so I'm sure the old method isn't going anywhere while that flaw remains. So long as the traditional patch files are supported I'll prefer to keep using them.

> appears to have several test failures.

Of the tested Drupal versions (10.1, 10.2, 11.x) only 10.1 seems to be currently passing its own tests (sans patches), and against that version this patch passed testing too. AFAICT the vast number of test failures on 10.2 and 11 are entirely unrelated to this patch, and there doesn't seem any point in re-testing until those branches start passing tests on their own.

> Also issue summary should follow standard issue template.

Indeed, that would be good.

The MR workflow at drupal.org continues to not provide persistent immutable patch URLs which is hugely problematic when it comes to referencing patches externally, so I'm sure the old method isn't going anywhere while that flaw remains. So long as the traditional patch files are supported I'll prefer to keep using them.

Please follow this issue #3412417: Disable DrupalCI testing → regarding the DrupalCI deprecation.

jweowu → changed the visibility of the branch 571548-identifiers-longer-than to hidden.

Based on that issue, I'm not confident that the 10.2.x and 11.x core branches are going to pass their traditional testing anytime soon, so I've made a MR for testing #180 on those versions.

Can you update to 11.x as the target. It can be backported on commit but has to land on 11.x first.

But see 10.2 is green so that’s good

Unless I'm missing something, the MR workflow more or less necessitates separate branches for testing against each version of Drupal as it's no longer a simple patch that's being applied, so I've created 10.2.x and 11.x branches for this.

Well, there's a MR test failure for "PHPUnit Functional Javascript 1/2" which recurred on a retry, but there's no Javascript involvement here, so I'm setting this back to Needs Review.

Hiding patches for clarity

Issue summary appeared to be updated in #183 so removing that tag

@jweowu the JS test failure is performance related, suggest to hit Retry on the job @ https://git.drupalcode.org/issue/drupal-571548/-/jobs/713304 if you have a button there for that (I don't).

I see the existing limit was already 60 characters on aliases, and I've read through the code replacing strtolower(substr($alias, 0, 60)); with similar code in a new method. To me it looks like existing alias behaviour should be preserved.

I did however wonder if any non-obvious changes in behaviour might impact sites which reference existing aliases that approach the limit, and which are handled differently after this change. For example, if a template referenced a long field name, that might require an update to the field in the template. Based on reading the code I don't think this is the case, but I want to mention it for consideration as that would be a breaking change?

We could also get rid of the is_string() check. Can the alias be anything else than a string?

Yes, at minimum the alias argument can be NULL. I can't recall offhand whether there are other possibilities, but there's at least that one, and the only thing the function can cope with is a string, so I think is_string() is the sensible guard.

For the open thread

I don't know why it auto-marked that as a draft, but I'd hazard a guess that it was complaining about the fixup commit (which I think is dumb, because this then means I need to force-push changes, rather than trivially squashing the fixups once it's reviewed and without imposing force-pushes on anyone). Anyhow -- no actual changes there.

That was just a phrasing tweak for the code comments.

Ran test-only feature so removing that tag

There was 1 error:
1) Drupal\Tests\field\Kernel\EntityReference\Views\EntityReferenceRelationshipTest::testNoDataTableRelationship
Error: Call to undefined method Drupal\views\Plugin\views\query\Sql::sanitizeAlias()
/builds/issue/drupal-571548/core/modules/field/tests/src/Kernel/EntityReference/Views/EntityReferenceRelationshipTest.php:144
/builds/issue/drupal-571548/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
ERRORS!
Tests: 5, Assertions: 111, Errors: 1.

Left 2 small comments on MR

Issue summary mentions a TODO to add steps to reproduce

Tagged for a simple change record for the new sanitizeAlias feature.

Believe this is super close

I wrote the steps to reproduce the issue and created draft change record.

Excellent, thank you sokru.

smustgrave → changed the visibility of the branch 571548-views-alias-truncation-10.2.x to hidden.

So #bugsmash is trying a new thing where we are focusing on a single component for a month and this month it's views, with this issue coming up.

The test-only feature fails as expected

1) Drupal\Tests\field\Kernel\EntityReference\Views\EntityReferenceRelationshipTest::testNoDataTableRelationship
Error: Call to undefined method Drupal\views\Plugin\views\query\Sql::sanitizeAlias()
/builds/issue/drupal-571548/core/modules/field/tests/src/Kernel/EntityReference/Views/EntityReferenceRelationshipTest.php:144
/builds/issue/drupal-571548/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
ERRORS!

This would make sense since the method doesn't exist yet so no real way to test that.

The issue summary appears very complete.

Reviewing the MR and new function are properly typehinted and comments.

I updated the CR to be more clear that a new function is being added.

Thanks for working on this old issue!

I read the IS, comments and the MR. The IS is easy to understand. For the comments it is interesting that a commit was made to Views when it was in contrib but more work is needed in core. daffie points out that migrate has the same problem with the length of table names. So, I refreshed my memory by looking at that MR.

The first thing I notice is that there are no comments on the code changes regarding the new method. And that there are no explicit tests of the new method. The existing functional tests is testing with just one string.

I left some comments and questions in the MR.