Problem/Motivation

From what I can tell, and I could be wrong, the condition for the string field type has a condition thats checked too early to allow multiple string fields on the same node to have their field content replaced in one run

Steps to reproduce

1. Fresh D10.2.4
2. Latest version of Scanner
3. Enable the Scanner module
4. Add 2/3 plain text, short fields to the basic page content type
5. Configure Scanner to use the new fields, all other settings leave as standard
6. Add a new basic page
7. Add the same text for all 3 of the fields added in step 4
8. Run a Scanner search for the string in step 7
9. Replace with something else

Expected: All 3 fields will be updated with the new string
Actual: Only the first field is updated

Proposed resolution

Move the condition that checks for a new vid to a similar position where it fires for text_long (etc)

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork scanner-3441892

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:

About issue forks

Comments

aaron.ferris created an issue. See original summary.

aaron.ferris’s picture

aaron.ferris commented
Title: Multiple string type fields on the same node require multiple replace » Multiple string type fields on the same node require multiple replace runs

aaron.ferris opened merge request !27

aaron.ferris’s picture

aaron.ferris commented
Status: Active » Needs review
aaron.ferris’s picture

aaron.ferris commented
Issue summary: View changes
damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Assigned: aaron.ferris » Unassigned

Thank you for opening the bug report and providing the merge request, I'll try to review it soon.

As a reminder, please update the "Assigned" field to "Unassigned" when you're finished, that way others know you're finished and they can take over.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Version: 8.x-1.0-rc7 » 8.x-1.x-dev
smustgrave’s picture

smustgrave commented
Version: 8.x-1.x-dev » 2.0.x-dev
Status: Needs review » Needs work

Needs a reroll for 2.0.x

smustgrave’s picture

smustgrave commented

Is eligible to be backported to 8.x-1.x so can do a 2nd MR.

aaron.ferris opened merge request !35

aaron.ferris’s picture

aaron.ferris commented

Added a reroll of this for 2.0.x that seems to resolve this for me.

aaron.ferris’s picture

aaron.ferris commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Can we add some test coverage?

Also does this need to be expanded to Paragraphs too?

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Issue tags: +Needs tests

Test coverage is definitely needed.

I think we should stick with nodes for this issue, and open a separate one if the change is also needed for paragraphs.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Parent issue: » #3577750: Plan for Scanner 2.0.0

Let's include this in 2.0.0.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Status: Needs work » Postponed (maintainer needs more info)

This should have been fixed in #3462717: Node logic only processes first value, looses other values in multi-value text fields, can you please test it with your scenario and let me know if the problem persists? Thank you.

grask0’s picture

grask0 commented

Hello,

I tested the latest DEV version of the module and got the following results.
I have a node with a title field and a field_code field (plain text). Both fields contain the exact same text.
When I search for the text, it only finds one match, which is in the field_code field.
When I try to replace the text, only the field_code field gets updated. The title field remains unchanged.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Status: Postponed (maintainer needs more info) » Needs work

Let's expand (and correct?) the test coverage, then see what needs fixing.

grask0 opened merge request !89

grask0’s picture

grask0 commented
Status: Needs work » Needs review
o_timoshchuk’s picture

o_timoshchuk commented
Assigned: Unassigned » o_timoshchuk
o_timoshchuk’s picture

o_timoshchuk commented
Assigned: o_timoshchuk » Unassigned
Status: Needs review » Reviewed & tested by the community

The merge request #89 fixed the problem.
I added three plain text fields to the Basic Page content type and created two nodes where the fields have identical values.
All field values were successfully searched, and their values were replaced as expected.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Status: Reviewed & tested by the community » Needs work

Thank you @grask0 for working through that and fixing the bugs!

A small request: Please expand the test coverage to cover multiple values on the same field, and multiple entities with the same value in the same field name.

grask0’s picture

grask0 commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Sorry multiple MRs can those that aren't for review be closed please

smustgrave closed merge request !35

grask0’s picture

grask0 commented

Hello @smustgrave,

#27 MR is related to the 8.x-1.x branch
#89 MR is related to the 2.0.x branch.

According to your comment https://www.drupal.org/project/scanner/issues/3441892#comment-15912794, 2 merge requests were prepared for both versions.
I'm not sure about #27 because I worked on #89 (which can be merged).

Thanks.

smustgrave closed merge request !27

smustgrave’s picture

smustgrave commented
Status: Needs work » Needs review

Fair, that was 2 years ago so think we don't have to worry about the 8.x-1.x branch anymore. Hopefully once we make 2.0.0 we can drop support.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Finally reviewed, the tests need to be updated to not use $this->entityStorage as that has been dropped due to new phpstan rules that discourage storing the storage object.

  • smustgrave committed 23b6e597 on 2.0.x authored by grask0 
    fix: #3441892 Search and replace when the same text appears in multiple...
smustgrave’s picture

smustgrave commented
Status: Needs work » Fixed

Fixed up.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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