Problem/Motivation

Currently the D7 tests are failing in testMultiValuedWidget():

    11 fail: [Other] Line 774 of modules/file/tests/file.test:
      8 fail: [Other] Line 754 of modules/file/tests/file.test:
      6 fail: [Other] Line 801 of modules/file/tests/file.test:
      5 fail: [Other] Line 821 of modules/file/tests/file.test:
      5 fail: [Other] Line 816 of modules/file/tests/file.test:

See: https://www.drupal.org/pift-ci-job/2318632

These failures seems to appear when there are multiple file fields in the node with a cardinality set to 2 or more (multiple values allowed). Testbot tries to upload files to these fields but after curl request one field does not generate upload form even if the cardinality is not reached..

Better explanation in images attached:

test_postgresql.jpg - causes test failure on PostgreSQL
test_postgresql

test_mysql.jpg - correct behavior on MySQL
test_mysql

I have assigned this as simpletest issue now, because when I try to simulate this manually on clean D7 install (in the UI, with JS disabled) all upload forms are visible correctly.

It is not important how many fields are there (testbot generates 2 fields, but the same result is when there are 3 or more) - the upload button remains only on the first field (event if others also should have it).

Steps to reproduce

Run Drupal 7 tests on PostgreSQL.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#7 3264750-7.patch3.21 KBmcdruid
#4 3264750-4.patch411 bytesmcdruid
#3 3264750-3.patch440 bytesmcdruid
#2 3264750-2.patch1.07 KBmcdruid
test_mysql.jpg84.5 KBpoker10
test_postgresql.jpg64.46 KBpoker10

Comments

poker10 created an issue. See original summary.

mcdruid’s picture

mcdruid commented
Status: Active » Needs review
StatusFileSize
new 3264750-2.patch1.07 KB

I've spent way more time digging into this than I should have done!

As far as I can see the problem is that the test is sending a POST to the form with multiple Upload buttons in a way that a real browser would not.

_form_builder_handle_input_element() tries to identify the triggering element:

https://git.drupalcode.org/project/drupal/-/blob/7.88/includes/form.inc#...

...and when the test sends its POST with a file, both field's upload buttons are identified as the triggering element.

The difference between PostgreSQL and the other dbs is the order that happens in; so when the first POST tries to upload to field2 it's the other upload button that is processed last and that gets set as the triggering element.

In MySQL (and presumably SQLite) field2's button is processed last and is therefore correctly set as the triggering element.

I'm not exactly sure what determines the ordering of the form elements; I'm guessing it's down to another db query without an ORDER BY that results in this different behaviour, but I've yet to track that down.

The same test already contains a workaround for there being multiple buttons with the same value on the form, and applying that to the Upload buttons seems to fix this (I must admit that I'm not 100% sure how the workaround works as I don't see how the $button array is actually in scope such that it affects the form processing; perhaps I'm missing something about how $this->xpath is working?) There are comments about the buttons workaround later on in the test - perhaps we should move that comment.

It would be good to understand what causes the form processing to work with a different order in PostgreSQL, but I think for now just fixing the test is okay. As mentioned I don't think a real browser would send the POST request that matches both buttons so the problem is specific to the test framework. @poker10 mentioned that this problem doesn't happen if you manually recreate the steps in the test, and I've confirmed that. It's pretty much down to luck that this test worked for the other databases AFAICS.

mcdruid’s picture

mcdruid commented
StatusFileSize
new 3264750-3.patch440 bytes

I think I've found the query lacking an ORDER BY.

This gets the test passing for me locally with pgsql - let's see if it breaks anything else.

If not, I think I'd vote for committing both of these patches.

mcdruid’s picture

mcdruid commented
StatusFileSize
new 3264750-4.patch411 bytes

Actually I think we only need one ORDER BY.

mcdruid’s picture

mcdruid commented

Oh dear the test results do seem quite inconsistent.

However, both of these patches fix FileFieldWidgetTestCase::testMultiValuedWidget

I think we should commit both; the test shouldn't be sending the malformed form submissions, and reading the field instances in a consistent order seems like an improvement too.

mcdruid’s picture

mcdruid commented
mcdruid’s picture

mcdruid commented
StatusFileSize
new 3264750-7.patch3.21 KB

Here's a patch with both fixes.

I've also tidied up the comments in the test, including adding an explanation of why modifications to each $button are passed through to drupalPost / handleForm, as it wasn't initially obvious to me.

poker10’s picture

poker10 commented

Thanks @mcdruid, very good job here! I have tried debugging this during the week also, but I was not able to track down the problem with a certainty.

I think that is a good idea to apply both fixes - especially if we want the consistency in select queries, where the returned order is something that the system relies on.

  • mcdruid committed 1ad0b4f on 7.x 
    Issue #3264750 by mcdruid, poker10: FileFieldWidgetTestCase::...
mcdruid’s picture

mcdruid commented
Status: Needs review » Fixed

RTBC from Fabianx in chat.

Status: Fixed » Closed (fixed)

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