CDPT-2259 Don't delete non-existent documents.

[EarthlingDavey]

This PR uses composer-post-install.sh to modify the ElasticPress plugin.

It adds a check for a document, before deleting it.

This will fix the following logs coming from the proxy service.

time="2024-11-08T15:32:26Z" level=error msg="error proxying request" message="{\"_index\":\"prod.bors\",\"_id\":\"432352\",\"_version\":1,\"result\":\"not_found\",\"_shards\":{\"total\":2,\"successful\":2,\"failed\":0},\"_seq_no\":79305,\"_primary_term\":2}" request="DELETE https://xyz.es.amazonaws.com/prod.bors/_doc/432352" status_code=404

[EmilyHazlehurst]

@EarthlingDavey is it possible to use one of EPs hooks for this instead of rewriting the code through a script? I haven't tested this, but there's ep_delete_{indexable_slug} which fires before deletion, for example https://10up.github.io/ElasticPress/ep_delete_%257Bindexable_slug%257D.html

[EarthlingDavey]

Hey @EmilyHazlehurst thanks for the suggestion. This might be one of the cases where there is no obvious 'best way'.

We only really need to do this get before delete for a specific code path and user action. action_sync_on_update is run when a user creates a new post/page etc. so on every new post we get this 404.

ep_delete_{indexable_slug} is later on in the code and at that point there is no way to know what the user action was.

I think if someone clicked the delete button we shouldn't be doing get before delete - a 404 in the logs if the document isn't there might be helpful.

I absolutely do see the benefit of using hooks and not writing custom hacks, so I'm on the fence. What do you think, I could put in a pull request upstream to add a filter in the ideal location - with the intent for us to use that if/when it's merged?

[EmilyHazlehurst]

Hey @EmilyHazlehurst thanks for the suggestion. This might be one of the cases where there is no obvious 'best way'.

We only really need to do this get before delete for a specific code path and user action. action_sync_on_update is run when a user creates a new post/page etc. so on every new post we get this 404.

ep_delete_{indexable_slug} is later on in the code and at that point there is no way to know what the user action was.

I think if someone clicked the delete button we shouldn't be doing get before delete - a 404 in the logs if the document isn't there might be helpful.

I absolutely do see the benefit of using hooks and not writing custom hacks, so I'm on the fence. What do you think, I could put in a pull request upstream to add a filter in the ideal location - with the intent for us to use that if/when it's merged?

Thanks for the info @EarthlingDavey.

Yeah, I'm really hesitant to add custom code in this way... I understand your points though, especially as we're then relying on a third party to merge. It just introduces a lot of potential headaches with updates/debugging later on.

I'm not sure if this is 'the Wordpress way', but is it possible to apply the change as a patch with composer? I usually use Composer Patches:

Repo: https://github.com/cweagans/composer-patches
Docs: https://docs.cweagans.net/composer-patches/getting-started/system-requirements/

You can then just add something like this to the composer file:

"enable-patching": true,
"patches": {
    "wpackagist-plugin/elasticpress": {
        "Bug description": "link/to/file or github.com/example.patch"
},

[EarthlingDavey]

Hey @EmilyHazlehurst composer patches looks interesting. I've used something similar with node.

Today's been very busy, so I'll check it out properly next week.

[EarthlingDavey]

Hey @EmilyHazlehurst I'm trying to weigh up the options and arrive at a pragmatic solution. Are these the main concerns that you had in mind:

• That, a plugin author could make changes, and an update would break the find/replace script, or even worse break the php script?
• Debugging is difficult because find/replace is a custom solution - and code is spread across multiple files.

Can you envisage other downsides to the find/replace script? All extra info will help us make an informed decision.

For additional context, we have had some success with creating a find and replace script, creating an upstream PR, and removing the find/replace once the upstream package released an update.

Debugging hasn't been much of an issue as the script runs locally, so the local files are an accurate representation of what's run, and what will be run upon deployment.

---

I've researched composer-patches and I'm not sure if it's improvements are significant enough to justify using it.

The pros are:

• It's a standardised approach to patching composer files.
• It's merge algorithm is probably better than a simple find and replace.
• It probably handles errors better than a find/replace script.

The cons are:

• It's an additional external dependancy - with a handful of sub-dependencies.
• There are a few outstanding issues.
• The last release was Dec '22, with the maintainer working on version 2, though I don't think it's released yet.

Do you think that's a fair summary, should I add anything else?

---

I'm wondering if there's a quick fix to improve the robustness of the find/replace script - so that it's on par with that of composer-patches.

Then we can postpone a decision to adopt composer-patches or something else.

[EmilyHazlehurst]

Hey @EmilyHazlehurst I'm trying to weigh up the options and arrive at a pragmatic solution. Are these the main concerns that you had in mind:

• That, a plugin author could make changes, and an update would break the find/replace script, or even worse break the php script?
• Debugging is difficult because find/replace is a custom solution - and code is spread across multiple files.

Can you envisage other downsides to the find/replace script? All extra info will help us make an informed decision.

@EarthlingDavey exactly that.

For additional context, we have had some success with creating a find and replace script, creating an upstream PR, and removing the find/replace once the upstream package released an update.

Debugging hasn't been much of an issue as the script runs locally, so the local files are an accurate representation of what's run, and what will be run upon deployment.

I suppose my concern here is how does someone know that the code has been changed with a find and replace. Is there something in the replaced code to indicate that it's been added by a script and where that script is?

Also, if something changes in the plugin how would someone know if the script failed to apply properly?

Say a new dev was asked to update the ElasticPress version. They run composer require x.y.z, would anything happen locally to indicate that there might be an issue? I'm guessing that's what the package check in bin/composer-post-install.sh is for? Maybe that file could have more information about the issues each find and replace is fixing/the process for updating a package with composer if there's an affected find and replace in there.

I've researched composer-patches and I'm not sure if it's improvements are significant enough to justify using it.

The pros are:

• It's a standardised approach to patching composer files.
• It's merge algorithm is probably better than a simple find and replace.
• It probably handles errors better than a find/replace script.

The cons are:

• It's an additional external dependancy - with a handful of sub-dependencies.
• There are a few outstanding issues.
• The last release was Dec '22, with the maintainer working on version 2, though I don't think it's released yet.

Do you think that's a fair summary, should I add anything else?

I'm wondering if there's a quick fix to improve the robustness of the find/replace script - so that it's on par with that of composer-patches.

Then we can postpone a decision to adopt composer-patches or something else.

That's fair. Given there's precedent for this in this project I don't mind approving this PR, but I agree it'd be good discuss the best solution going forward as part of tech. maintenance 🙂

[EarthlingDavey]

Hey @EmilyHazlehurst , thanks for the extra detail 💯 I've made some final changes based on your suggestions.

I suppose my concern here is how does someone know that the code has been changed with a find and replace. Is there something in the replaced code to indicate that it's been added by a script and where that script is?

This is such a good shout, thanks! In this case I've now added a comment

// The following line was modified by composer-post-install.sh

if something changes in the plugin how would someone know if the script failed to apply properly

The verify_composer_package_version will throw an error if the version doesn't match, I checked in the wrong version number and this is caught by GitHub's PR checks.

Maybe that file could have more information about the issues each find and replace is fixing/the process for updating a package with composer if there's an affected find and replace in there.

I've added a comment to to the script file, that should help debugging and identifying when the find/replace is no longer needed.