- Issue created by @aurora-norris
- 🇬🇧United Kingdom aurora-norris
I can fix it by patching core with the fix for https://www.drupal.org/i/2925972 → but I feel like there should be a less invasive way of fixing this.
- 🇬🇧United Kingdom aurora-norris
I've found better way to fix it, if just before you set the new field you get the current field and compare that both
GeolocationItem
will have data stored as floats and because they get their data from the same source they are exactly equal.
A maintainer thinks this is a good way to solve this issue then I can make a merge request. - 🇮🇹Italy itamair
Hi @aurora-norris thanks for reporting and inspecting all this, so deeply.
Actually I am already inspecting and debugging this ... and indeed it looks that the GeocoderField @QueueWorker $entity->save() here:
https://git.drupalcode.org/project/geocoder/-/blob/8.x-4.x/modules/geoco...
is retriggering a new geocoder_field_entity_presave, at least once ... and then not a further time, as it looks the second time the $result === FALSE and then the $entity is not being resaved recursively ...Thus I was thinking we should mark/flag the saved $entity in some way as saved by the @QueueWorker and stop all the new geocoder_field_entity_presave in that case.
I am going to inspect and implement this approach.
What you think?Feel free to open your MR if you ended up to a better fix, so that I could review it, and compare, eventually.
Feel also free to PM me (itamair) on Drupal slack for more immediate feedback. - Merge request !70Set drupal static cache so to mark the entity as being saved by the geocoder... → (Merged) created by itamair
- 🇮🇹Italy itamair
@aurora-norris it looks that I was quicker and opened a MR !70 with a solution that relies on drupal_static cache so to mark the entity as being saved by the geocoder queue and stop any further processing in the geocoder_field_entity_presave.
That looks pretty logical and solid to me, and looks properly fixing this issue and stop further redundant entity save and revisions generations.
Please QA and Review ... and let me know if some more work is needed on this (and why eventually).
- 🇬🇧United Kingdom aurora-norris
@itamair
I've tested your merge request and it fixes my issue and processes the addresses that were being resaved every cron run.
It looks like the better and more correct solution compared to mine.The code looks good and I've traced what happens on a normal entity save (where the entity is queued) and on a save by the QueueWorker and the new code does exactly what its meant to do.
I'll make a MR with my solution in a minute so you can compare them because both our solutions issue at different levels.
- Merge request !71Fetch the field from the entity again and compare it to the new field to make... → (Open) created by Unnamed author
- 🇬🇧United Kingdom aurora-norris
Made a MR with my solution, feel free to not use.
- 🇮🇹Italy itamair
Closing this as Fixed, with MR70! merged. Thanks @aurora-norris ... crediting you for this opportune catch.
- 🇩🇰Denmark ressa Copenhagen
This was a beautiful issue to find by chance, while checking changes in the latest release -- both the great detective work and thorough issue report with plenty of details by @aurora-norris, and @itamair coding and releasing a solution in less than a day. It's quite impressive, and I am very grateful to both of you.