jhedstrom / DrupalDriver

A collection of drivers for controlling Drupal.
GNU General Public License v2.0
64 stars 97 forks source link

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

Open daggerhart opened 3 years ago

daggerhart commented 3 years ago

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 |
nicrodgers commented 3 years ago

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.

mdolnik-eelzee commented 3 years ago

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:

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

chrisolof commented 3 years ago

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.

BramDriesen commented 3 years ago

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

BramDriesen commented 3 years ago

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