DrupalDriver icon indicating copy to clipboard operation
DrupalDriver copied to clipboard

Fix for address fields when value is an associative array of address field keys

Open daggerhart opened this issue 3 years ago • 5 comments

Having the same issue as #206 (Undefined offset 0) when trying to create nodes with address fields. The AddressHandler did expect keyed values matched the keys for the address field definition.

I tried the various formats for address fields listed here, but none of them worked.

This change allows for the following formats:

Given location content:
  | title | field_address:address_line1 | :locality | :postal_code |
  | SC 1  | 1111 Main St                | My City1  |  11111       |
  | SC 2  | 1112 Main St                | My City2  |  11112       |

And location content:
  | title | field_address                                                         |
  | SC 3  | address_line1: 1113 Main St - locality: My City3 - postal_code: 11113 |
  | SC 4  | address_line1: 1114 Main St - locality: My City4 - postal_code: 11114 |

daggerhart avatar Jun 06 '21 12:06 daggerhart

I upgraded from 1.3 to 2.1 yesterday and suddenly all my scenarios that contained steps like this started failing:

Given "location" content:
      | title                            |field_address:country_code | :organization | :address_line1 | :address_line2 | :postal_code | 
      | BEHAT - Non Child care RISCA     | GB                        | orgname       | address1       | address2       | NP15 1TR     | 

(where field_address is a Drupal 8 core Address field)

I can't see why AddressHandler is needed. If I delete the AddressHandler.php file, everything works fine.

I did try this pull request but it made no difference.

nicrodgers avatar Jun 08 '21 09:06 nicrodgers

So there seems to be quite a bit wrong with the existing AddressHandler class:

  1. It does not support setting sub-fields via named keys (as this PR is addressing).
  2. It does not support multi-value address fields, the $return array will overwrite all previous values in the passed in $values array to essentially ignore multi-values and set it to a single value.
  3. While it only supports adding values in numeric order (eg 1234 Main St - My City - 43210) this also relies on knowledge of the configuration of the field itself:
    • ie: With the example 1234 Main St - My City - 43210, if both Address lines 1 and 2 are present in the field, My City will end up in Address Line 2 and the postal code will end up in City.

I agree with @nicrodgers in that it would probably be more ideal to just remove this class as it breaks in the situations I've listed, and it also goes against the address example in the field_handlers.feature file that @daggerhart pointed out above.

That-said, this class has been out in the wild since v2.0.0-alpha4 and any usage of it will have people writing tests with the values in numeric order rather than using keyed values.

So we should probably allow for both cases of keyed and numeric entries, while fixing any of the issues.

While the code in this PR does work, it still has the issues:

  • Still does not support multi-value address fields.
  • It will fail unless ALL of the address sub-fields are provided:
    • eg If the address field is configured to have the sub-fields (address_line1, postal_code, locality) and only address_line1 and locality are provided, we still get the same Undefined offset 0 error.

I have provided the following patch which I believe solves all of these issues, feel free to use this in the PR and make any changes if needed: https://gist.githubusercontent.com/mdolnik-eelzee/c18df4dfe2d9338d2a81afa4240c6f08/raw/fe606f740a43a0739066956e4e212cff0bb9f483/AddressHandler_236.patch

mdolnik-eelzee avatar Jun 15 '21 18:06 mdolnik-eelzee

I just tried @mdolnik-eelzee's patch and I can confirm that it does work in my project. @mdolnik-eelzee, would you mind opening a new PR, referencing #206, with your proposed changes? I have a feeling that will have a better chance at getting merged in.

chrisolof avatar Oct 25 '21 21:10 chrisolof

@chrisolof I created a pull request with the patch provided by @mdolnik-eelzee

BramDriesen avatar Oct 29 '21 14:10 BramDriesen

I think we can close this one in favour of #240 everything is linked together so the conversation will not be lost 😃

BramDriesen avatar Oct 29 '21 14:10 BramDriesen