Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

daggerhart
Copy link

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 daggerhart force-pushed the 206--address-field-keyed-values branch from 00d2161 to 441cf6b Compare June 6, 2021 12:07
@nicrodgers
Copy link

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
Copy link

mdolnik-eelzee commented Jun 15, 2021

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

@chrisolof
Copy link

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
Copy link

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

@BramDriesen
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants