🇺🇸United States @bkline

@davidwbarratt

This problem seems exceptionally bad with SQLite.

That would seem to imply that you are able to reproduce the unwanted behavior using this patch with other databases than SQLite (just not as frequently). Can you confirm that this is true?

@jcandan I absolutely agree that it is logical for a later ticket to supersede an earlier ticket. I just don't agree that "supersedes" and "duplicates" are synonyms.

I'm sorry, how can an issue duplicate a second issue which didn't even exist until seven years after the first issue was created?

@larowlan (#15) Sorry for the delayed response. Yes, I believe it's true that Drupal provides no way to prevent the return of values entered/selected on one path when the user changes his/her mind and selects another path (a mythical #forbidden mirror image of #required), and it's also true that (short of reloading the form and starting over) the user can't back out of having a selection for a set of radio buttons once a selection has been made. But that just means that the submit handler just has to be intelligent about what it needs, based on the state of the field which determines which path was taken by the user when the form was submitted. In other words, it's not a big deal that the handler might get more than it needs, but it is a problem that it might get fewer values than are needed (and that the rendering of the form fails to convey the fact that the field is required).

Sure. I have switched the component to "entity system" (don't see an option for "entity query" so I assume it's lumped in with entities in general). I'll edit the issue description. If what you say is true (i.e., the API is broken), then there will be two "remaining tasks" instead of just one:

1. Finish the documentation
2. Fix the code

In our own software, that's the order in which we like to do things. If you fully document what the code is supposed to do, it's much easier to write the code, as well as know when the code is correct. 😉

I don't think condition is a good example of what the syntax might be.

Well, narrowing a query by the value of a field, aggregating the values of a field, and sorting based on the aggregated values of a field all share the requirement that the programmer has to answer the same question: "Which field?", right? If one follows the age-old principle of not multiplying entities beyond necessity, it would seem reasonable to use the same syntax for meeting this requirement. It's possible that there is some valid reason why that won't work here, and that a different syntax is needed for at least one of these actions, but I'd want to see compelling arguments to that effect before I'd be convinced. In any case, the required syntax needs to be documented, and that would be even more true if the surprising decision was made to use different syntax for these similar operations.

Do you agree?

Seem like the issue tracker is itself having issues. I tried editing a typo in my previous comment and I got a server error message (see second attached screenshot). Second attempts for each failure worked. 🤔

Well that was strange. As you can see from the screenshot, I add attached the repro code file before submitting my previous comment, but the issue tracker discarded it. Trying again.

OK, I've got a script you should be able to run yourself. The SQL error message is not the same, complaining about an unknown column instead of a syntax error (likely because my original repro case had multiple levels of nested entity references, whereas this has a single level), but I believe it adequately illustrates what this issue is reporting, namely that the documentation does not provide enough information to know exactly what the method expects for the first argument in the general case. The code is attached (inside a zipfile, as this interface won't allow me to post a PHP script directly).

root@8fca212939df:/var/www# vendor/bin/drush scr --script-path=/var/www/bug-repros repro-3282364-2 
created tag with ID 206
created tag with ID 207
created tag with ID 208
created article with ID 52
created article with ID 53
52: Tag for bug 3282364 (1)
53: Tag for bug 3282364 (3)
failure: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'field_tags.entity.name_max' in 'order clause': SELECT "node_field_data_2"."nid" AS "nid", MAX("taxonomy_term_field_data"."name") AS "name", MAX("taxonomy_term_field_data_2"."name") AS "field_tagsentityname_max"
FROM
"node" "base_table"
LEFT JOIN "node__field_tags" "node__field_tags" ON "node__field_tags"."entity_id" = "base_table"."nid"
LEFT OUTER JOIN "taxonomy_term_data" "taxonomy_term_data" ON "taxonomy_term_data"."tid" = "node__field_tags"."field_tags_target_id"
LEFT JOIN "taxonomy_term_field_data" "taxonomy_term_field_data" ON "taxonomy_term_field_data"."tid" = "taxonomy_term_data"."tid"
LEFT OUTER JOIN "taxonomy_term_data" "taxonomy_term_data_2" ON "taxonomy_term_data_2"."tid" = "node__field_tags"."field_tags_target_id"
LEFT JOIN "taxonomy_term_field_data" "taxonomy_term_field_data_2" ON "taxonomy_term_field_data_2"."tid" = "taxonomy_term_data_2"."tid"
INNER JOIN "node_field_data" "node_field_data" ON "node_field_data"."nid" = "base_table"."nid"
INNER JOIN "node__field_tags" "node__field_tags_2" ON "node__field_tags_2"."entity_id" = "base_table"."nid"
LEFT JOIN "node_field_data" "node_field_data_2" ON "node_field_data_2"."nid" = "base_table"."nid"
WHERE ("node_field_data"."status" = :db_condition_placeholder_0) AND ("node__field_tags_2"."field_tags_target_id" IS NOT NULL)
GROUP BY "node_field_data_2"."nid"
ORDER BY "field_tags"."entity"."name_max" DESC; Array
(
    [:db_condition_placeholder_0] => 1
)

cleaning up term 206
cleaning up term 207
cleaning up term 208
cleaning up article 52
cleaning up article 53

We have millions of entities in this application, but only a small number of those are instances of a core type, and none of those are used in an aggregation query. Let me see what I can come up with either using just core types (or custom types created in the repro code).

Yes, here's the code. I thought I had made it clear in the original issue description what string I tried for identifying the values to be used for the sort.

$storage = \Drupal::entityTypeManager()->getStorage('ebms_packet');
$query = $storage->getAggregateQuery()->accessCheck(FALSE);
$query->condition('active', 1);
$query->exists('articles.entity.reviews');
$field = 'articles.entity.reviews.entity.posted';
$alias = 'updated';
$query->aggregate($field, 'MAX', NULL, $alias);
$query->sortAggregate($field, 'MAX', 'DESC');
$query->groupBy('id');
foreach ($query->execute() as $values) {
    echo "{$values['id']}: {$values['updated']}\n";                             
}                                                                               

And when I run vendor/bin/drush scr /var/www/bug-repros repro-3282364 I get

In ExceptionHandler.php line 52:
SQLSTATE[42000]: Syntax error or access violation: 1046 You have an error in your SQL syntax ...

If I comment out the sortAggregate() line, the query completes successfully, but of course the results aren't sorted correctly.

Even if you're saying that we can't expect the documentation to explain how to use the APIs correctly, and are instead supposed to dig into test code to glean the missing information (and I hope that's not what you're saying), the test code to which you linked handles the easy case of entities with primitive fields, but doesn't have anything for the more common case of custom entity types with structured values using entity references. That's why (as I explained in the original issue description) that I had to fall back on investigating methods whose documentation wasn't so sparse. Unfortunately the condition() method and the sortAggregate() method don't use the same syntax for identifying a structured field.

Here's an illustrative example for the structure of the custom entities involved:

> \Drupal\ebms_review\Entity\Packet::load(17687)
= Drupal\ebms_review\Entity\Packet {#7156
    translationLanguages: "und",
    id (integer): "17687",
    topic (entity_reference): "Topic::load(18)",
    created_by (entity_reference): "User::load(400)",
    created (datetime): "2019-12-31 08:12:16",
    title (string): "Childhood Acute Lymphoblastic Leukemia (January 2020)",
    articles (entity_reference) x2: "PacketArticle::load(63), PacketArticle::load(104)",
    reviewers (entity_reference) x6: "User::load(142), User::load(156), User::load(165), User::load(170), User::load(192), User::load(340)",
    last_seen (datetime): "2020-02-22 12:43:43",
    active (boolean): "1",
    starred (boolean): "0",
    summaries (entity_reference): "Doc::load(8573)",
    reviewer_docs (entity_reference): [],
  }

> \Drupal\ebms_review\Entity\PacketArticle::load(63)
= Drupal\ebms_review\Entity\PacketArticle {#6153
    translationLanguages: "und",
    id (integer): "63",
    article (entity_reference): "Article::load(650479)",
    dropped (boolean): "0",
    archived (datetime): [],
    reviews (entity_reference) x3: "Review::load(69422), Review::load(69570), Review::load(69382)",
  }

> \Drupal\ebms_review\Entity\Review::load(69422)
= Drupal\ebms_review\Entity\Review {#7399
    translationLanguages: "und",
    id (integer): "69422",
    reviewer (entity_reference): "User::load(142)",
    posted (datetime): "2020-01-23 16:34:01",
    comments (text_long): [
      [
        "value" => null,
        "format" => null,
      ],
    ],
    loe_info (string_long): [],
    dispositions (entity_reference): "Term::load(98)",
    reasons (entity_reference): [],
  }

If you still need more information, please ask. And let's use the "needs more information" status instead of "can't reproduce" in that case.

bkline → created an issue.

bkline → created an issue.

Is there a good reason why this documentation shouldn't be clearly marked as applicable to all versions of Drupal newer than version 7?

Added a link to the Update API.

I replaced the line

  $schema = $schema_property->getValue($field_storage_definitions[$field_name]);

with

  $schema = $field_storage_definitions[$field_name]->getSchema();

because (as @james.williams explained in a Slack thread), the schema property is only populated as and when needed, so it could be null at that point. The original code failed in our update hook, and the replacement succeeds.

We're on 10.2.3 now, and xdebug is broken again. I don't know that it's the same problem, though, because the errors in the JS console are different.

GET http://ebms.localhost:8081/sites/default/files/js/js_p1j4ni2gn90-X7Lhdu3LzIVHFMfLWWn5ntPOrtwj-Fk.js?scope=footer&delta=0&language=en&theme=ebms&include=eJx9T0ESwyAI_JCJb-hLMoi0caLBEUza3zcmtYceeoFdZlkW5FXpqRWi9aVmiCN-J0MM6yJGXqKUrAMhU2X3MjVo7wUS7VwWQy6JBRFSuTBWUU4GuVB3BdSw0el4aqaZE9lWLnpqwaewGmWODor9dONpoyNdq0OfVaHSrRsewxFbDP77pu_KzEWxapd0_nt4JEHIdDtDufCYcshkO3gDz8153g net::ERR_ABORTED 500 (500 Service unavailable (with message))
ebms.localhost/:1 Refused to execute script from 'http://ebms.localhost:8081/sites/default/files/js/js_p1j4ni2gn90-X7Lhdu3LzIVHFMfLWWn5ntPOrtwj-Fk.js?scope=footer&delta=0&language=en&theme=ebms&include=eJx9T0ESwyAI_JCJb-hLMoi0caLBEUza3zcmtYceeoFdZlkW5FXpqRWi9aVmiCN-J0MM6yJGXqKUrAMhU2X3MjVo7wUS7VwWQy6JBRFSuTBWUU4GuVB3BdSw0el4aqaZE9lWLnpqwaewGmWODor9dONpoyNdq0OfVaHSrRsewxFbDP77pu_KzEWxapd0_nt4JEHIdDtDufCYcshkO3gDz8153g' because its MIME type ('text/html') is not executable, and strict MIME type checking is enabled.

Confirming that it works as documented again with the patch.

We understand that eventually (that is, before we get to D11) we need to replace

          $validators = ['file_validate_extensions' => ''];

with

          $validators = ['FileExtension' => []];

(or whatever the documentation for the replacement for the soon-to-be-deprecated file_save_upload() function tells us to use).

Thanks!

I'll test in the morning (EST).

Using your patch to avoid blowing up on the array_unshift() call, I'm now getting "Only files with the following extensions are allowed: jpg jpeg gif png txt doc xls pdf ppt pps odt ods odp." I see that your test uses a file with a name ending in ".txt" which is one of the extensions on the default list, whereas my test with an extension not on the list fails, meaning the advice in the documentation no longer works (that is, it doesn't achieve the goal of allowing any filename extension). What happens if your test uses a filename like "aaa.bbb"?

Let me know if you're not able to get to the GitHub link I provided, and I'll come up with another way to provide you with our custom code.

Sorry. I assumed when I gave the exception class (TypeError) and the method where array_unshift() is invoked (FileValidator::validate()) you'd have enough information. It's no surprise that array_unshift() is unhappy when the first argument isn't an array. 😉

Here's the exception:

[Mon Feb 12 06:57:09.773233 2024] [php:notice] [pid 788] [client 192.168.65.1:24832] Uncaught PHP Exception TypeError: "array_unshift(): Argument #1 ($array) must be of type array, string given" at /var/www/web/core/modules/file/src/Validation/FileValidator.php line 49, referer: http://ebms.localhost:8081/articles/import/27530

In this case, the apache logs don't get the full stack trace, so I've captured the call stack from the debugger. This is right before array_unshift() is called with $options set to an empty string:

and here is the call stack immediately after the exception is thrown:

Here's the custom code calling file_save_upload():

          $validators = ['file_validate_extensions' => ''];
          $file = file_save_upload('file', $validators, FALSE, 0);

I'm not 100% certain you'll be able to open this GitHub URL, as our client has recently added an additional login requirement, which sometimes blocks access and sometimes doesn't (I haven't yet completely figured out the pattern):

https://github.com/NCIOCPL/ebms/blob/main/web/modules/custom/ebms_import...

bkline@rksystems.com → created an issue.

Sigh. Oh, well, at least they're just deprecation messages, so they be gone eventually, along with the incorrect wording. 🙄

@kim.pepper Your description in #20 matches what I assumed you meant the deprecation message to convey, but that's not what it actually says. If you read its language carefully, you'll realize that "Calling" is the subject of both verbs, so parsed correctly, the meaning is as follows:

"Calling __construct() without the $lock argument is deprecated in drupal:10.3.0."

and

"Calling __construct() without the $lock argument is required in drupal:11.0.0."

@trigger_error('Calling ' . __METHOD__ . '() without the $lock argument is deprecated in drupal:10.3.0 and is required in drupal:11.0.0. See https://www.drupal.org/node/3389017', E_USER_DEPRECATED);

I don't think this deprecation message means what it says, which is that calls to the FileUploadHandler constructor will be required to omit the $lock argument in drupal:11.0.0. There are a number of spots like this in the patch.

Perhaps you meant "prohibited" or "prevented" where you have "required"?

Did you not see my request for clarification of what you were asking for in my last comment? And can you explain why it is by design that radio buttons which work in the Stark theme would fail with this theme?

This failure was resolved with the upgrade to D10.1.1 or D10.1.2. I don't know which because we had uninstalled xdebug when we ran into this bug, and only reinstalled xdebug with D10.1.2. This ticket can be closed, unless someone else is still experiencing the failure with the latest release of Drupal.

#23 None of which I'm aware. What did you have in mind? This issue assumes that the implication in the module documentation guidelines (that the use of markdown for such documentation is appropriate) is correct.

I had no idea when I pointed out the discrepancy five years ago that it would turn out to be so controversial. 😉

@berdir This issue was marked as a duplicate, but I can't find any identification of the other ticket of which this one was deemed to be a duplicate. Is it there and I just missed it?

Can you clarify exactly what you'd like me to try? I'm not sure I understand what "verify the test is pulling the theme in" would mean beyond what the first screenshot has already done by showing the footer which can only have come from uswds_base. To me that seems like pretty strong evidence that the theme has been "pulled in."

In the repro code above (pared down to the minimum), yes. In our actual test, no. For that the accordion is closed on page load, so we have a line

  $form->findButton('Display Options')->click();

When the next line creates and save the screenshot, the accordion is open.

I don't think so. I did, however, get some insight from Conrad Lara (@cmlara) in the Slack #support channel. Apparently Drupal sometimes (but not always) serializes the instance of the FormBase-derived class and then reconstitutes most of it, invoking submitForm() on this partially-resurrected instance. In the process it drops the private member variables on the floor. "Sometimes" likely means "when AJAX is in play" in this context. As far as Conrad is aware, this behavior isn't documented anywhere. But sure enough, when I replace private with protected in the repro code I posted the form works correctly. I will edit the original issue summary to clarify that it's private member variables which are discarded.

I have provided a repro case demonstrating the steps given above.

bkline@rksystems.com → created an issue.

I get the argument that it's not worth fixing, but unless someone can point to documentation which explicitly states that the tracker system was intentionally designed to behave this way, I think "Closed (won't fix)" is a more appropriate status for this issue.

bkline@rksystems.com → created an issue.

Here's a screen shot showing the position of the new field.

I'm a little bit puzzled by your request (and the new state for the issue), since I provided steps to reproduce the problem with the original ticket, and included a link to the code which demonstrates the problem. Can you be more specific about what you're asking for? Other than substituting our module name for "hook" in the name of the function, and a renamed variable, our code is pretty much identical to the code I linked to in the hook's documentation.

/**
 * Implements hook_ckeditor5_plugin_info_alter().
 */
function ebms_core_ckeditor5_plugin_info_alter(array &$plugin_definitions): void {
  assert($plugin_definitions['ckeditor5_link'] instanceof CKEditor5PluginDefinition);
  $definition = $plugin_definitions['ckeditor5_link']->toArray();
  $definition['ckeditor5']['config']['link']['decorators'][] = [
    'mode' => 'manual',
    'label' => 'Open in a new tab',
    'defaultValue' => TRUE,
    'attributes' => [
      'target' => '_blank',
    ],
  ];
  $plugin_definitions['ckeditor5_link'] = new CKEditor5PluginDefinition($definition);
}

bkline@rksystems.com → created an issue.

bkline@rksystems.com → created an issue.

@cilifen Thanks, I started to pick asset library system and then I saw aggregator.module and fell into the trap. 😂

@Chi Bumped up the max_nesting_level value to 4096. Same failure.

bkline@rksystems.com → created an issue.

Our users just asked for this feature. Can you provide a clue as to what that one line of JS would be?

Confirming patch #16 works on D10.1.0 with linkit module.

bkline@rksystems.com → created an issue.

bkline@rksystems.com → created an issue.

bkline@rksystems.com → created an issue.

bkline@rksystems.com → created an issue.

I just looked at the Dockerfile, and it's hard-wired to amd64. Is that all that's going to be supported?

[I started to follow the instructions which said "You can log in and create a Documentation issue" but when I followed that link I saw that issues were only intended for major issues, so I doubled back to this form.]

This page has instructions for setting up a separate Docker container for chromedriver, but it doesn't explain how that container is going to be able to connect back to the test web server, which is in a separate container. As far as I can tell the test web server is only listening for connections from localhost, with no way that I could find to override that to have it listen on 0.0.0.0 (or at least the local Docker network). Could someone who understands how that works please fill in the gap? Thanks.