Problem/Motivation

#3384140 introduced a regression of sorts; filtering in the NameWidget uses array_search() only returns the first key:

If needle is found in haystack more than once, the first matching key is returned. To return the keys for all matching values, use array_keys() with the optional filter_value parameter instead.

https://www.php.net/manual/en/function.array-search.php

So, if we have both title and generational in the form and the user leaves both blank, only one of these is going to get massaged to remove the "_none".

Steps to reproduce

Use a name field with multiple selects (e.g. title, generational) and leave both unassigned. When saving, only one of the values will be massaged and the other will have the literal "_none" persisted to the database.

Proposed resolution

Proposed patch attached

Issue fork name-3491471

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

tkiehne created an issue. See original summary.

tkiehne’s picture

tkiehne’s picture

Title: Literal "_none" value saved for seelct fields » Literal "_none" value saved for select fields

sourabhsisodia_ made their first commit to this issue’s fork.

sourabhsisodia_’s picture

Assigned: Unassigned » sourabhsisodia_
sourabhsisodia_’s picture

StatusFileSize
new120.41 KB
new121.38 KB

Attaching screenshot for reference.

sourabhsisodia_’s picture

Assigned: sourabhsisodia_ » Unassigned
Status: Active » Needs review
heddn’s picture

Status: Needs review » Needs work

Feedback posted on the MR.

sourabhsisodia_’s picture

Status: Needs work » Needs review
alan d.’s picture

I cranked up my email account linked to drupal.org and saw this thread...

I can't remember the sequence of the field widget submission processing, but can someone manually check that the validation in a required textfield component still triggers if you enter _blank for that component? If it does, then this minor fix may need a tweak to only target option based components.

heddn’s picture

Status: Needs review » Needs work

Feedback posted on the MR.

sourabhsisodia_’s picture

@heddn In required textfield components i have tested that when we write _none then validation error is not triggering and _none is getting replaced by "" (empty strings) . When we write _blank then also there is no validation error and the field is having _blank value.

sourabhsisodia_’s picture

Status: Needs work » Needs review
heddn’s picture

Status: Needs review » Needs work

I think this could still add some more edge cases to the tests to address #11.

megachriz made their first commit to this issue’s fork.

megachriz’s picture

To address #11, I've added test coverage for using "_none" as value for text fields. And the tests are failing. If you for example enter "_none" for the field "Family" an empty value gets saved for that field. If the name field is required you get the more cryptic error message "This value should not be null.".

megachriz’s picture

Status: Needs work » Needs review

I've added a possible fix for the case from #11.

sayan_k_dutta’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed the MR. Checked all the changes made.
1. Checked that on submitting more than one empty components for the name field, all the "_none" values are replaced by empty strings.
2. Checked that the validation does not trigger if we give "_blank" or "_none" as input.
3. Checked all the tests written and all of them passed.

Hence moving it to RTBC.

penyaskito’s picture

Had this same issue when leaving title and generational components empty (only if BOTH were empty).
This MR fixed the issue.

As I'm using the name output as the entity title, had to write an upgrade path in my project. If it helps, customize your entity type id and field name:

function mymodule_update_8001(&$sandbox): void {
  $nids = Database::getConnection()->select('node_revision__FIELD_NAME', 'name')
    ->fields('name', ['entity_id'])
    ->condition('FIELD_NAME_generational', '_none')
    ->execute()->fetchAll(\PDO::FETCH_COLUMN, 0);
  Database::getConnection()->update('node__FIELD_NAME')
    ->fields(['FIELD_NAME_generational' => ''])
    ->condition('FIELD_NAME_generational', '_none')
    ->execute();
  Database::getConnection()->update('node_revision__FIELD_NAME')
    ->fields(['FIELD_NAME_generational' => ''])
    ->condition('FIELD_NAME_generational', '_none')
    ->execute();
  \Drupal::entityTypeManager()->getStorage('node')->resetCache($nids);
}

brad.bulger’s picture

I am testing this using the MR diff as a patch on Name 1.0. We have code that runs in an ajax callback function that builds a fullname string to store in another field. $form_state->getValue('field_name') is returning "_none" in the array of components. So the name.formatter service's format() function treats those like strings - we get "_none John Doe _none".

Is this the intended behavior? Anyone else who was counting on empty components returning empty strings is going to have a similar incompatibility problem.

penyaskito’s picture

@brad.bulger Check my last comment where I posted a workaround for a similar usecase. Check if _none already hit the database before adding this patch.

brad.bulger’s picture

This is an AJAX callback in the form that is getting the name field value at runtime. Nothing is being written to the db. The relevant part is that $form_state->getValue('field_mynamefield') returns "_none" as the value for select list components.

What I'm not clear on is if that's intended. Does the change here only acts on submit or something like that. Should the value be "_none" or "" for field validation? For an entity constraint on the field?

I guess the change from blank to "_none" already went out in 1.0, I don't see any notes about the incompatibility change.

penyaskito’s picture

I'd say that's expected from $form_state. You'd need to access the widget and call extractFormValues.

liam morland’s picture

texas-bronius’s picture

Thank you! Did not extensively test but confirmed patch from https://git.drupalcode.org/project/name/-/merge_requests/24 working in my use case.

bkosborne’s picture

Priority: Major » Critical

Bumping to critical as this can lead to data issues. The MR works well for me.

clairero’s picture

Thanks, this MR resolves the issue for me too.

bluegeek9 made their first commit to this issue’s fork.

bluegeek9 changed the visibility of the branch 8.x-1.x to hidden.

jlancaster’s picture

Confirming MR24 works fine in multiple production environments. Would be nice to get this merged into an official release. In my case, the literal _none value translates to auto_entitylabels generating label values with an unintended "_none" inserted.

heddn’s picture

Status: Reviewed & tested by the community » Fixed

Thanks everyone for their contributions on this issue.

Status: Fixed » Closed (fixed)

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