Add filter for shiny updates

[afragen]

If a plugin that updates via shiny updates isn't hosted in the repo the destination and destination names may need to be renamed. This is done via whatever updating code a non-dot-org hosted repo uses.

If plugin updates are done via shiny updates the version info won't be found as the $result in wp_ajax_update_plugin() is looking in a destination that has been renamed.

Adding a filter would allow for other updaters to add the correct data to $result

Use case:
When using GitHub Updater an update will occur with shiny updates but the version info is replaced with an empty string. Adding a filter allows me to pass the corrected data to shiny updates.

I've added a patch but I apologize if I'm not following any specific filter naming convention.

[bordoni]

A big +1 for this Filter!

[afragen]

update DocBlock for filter

[dd32]

If plugin updates are done via shiny updates the version info won't be found as the $result in wp_ajax_update_plugin() is looking in a destination that has been renamed.

This use-case should be fixed through the existing code, rather than through a filter. I say this as it's supported in the standard code, and I thought I'd tested it as working (but evidently not).

What would be another use-case for this potential filter?

[afragen]

Replying to dd32:

If plugin updates are done via shiny updates the version info won't be found as the $result in wp_ajax_update_plugin() is looking in a destination that has been renamed.

This use-case should be fixed through the existing code, rather than through a filter. I say this as it's supported in the standard code, and I thought I'd tested it as working (but evidently not).

What would be another use-case for this potential filter?

There's no ability to fix this through existing code as the renaming occurs in either the upgrader_post_install or upgrader_source_selection filter(s).

In the referenced function wp_ajax_update_plugin() a new Plugin_Upgrader instance is created and the $upgrade->bulk_upgrade() doesn't seem to have a filter for it's return value.

If there's a filter available to modify the results of this function I'm not sure what it is.

[dd32]

There's no ability to fix this through existing code as the renaming occurs in either the upgrader_post_install or upgrader_source_selection filter(s).

Then we need to fix that, simple :)

It's possible to upgrade a WordPress.org-hosted plugin which was originally in a different folder than it's w.org slug too, so it's not a unique situation at all - it's one that is intended to be supported.

[afragen]

Replying to dd32:

There's no ability to fix this through existing code as the renaming occurs in either the upgrader_post_install or upgrader_source_selection filter(s).

Then we need to fix that, simple :)

Simple for whom? :)
One day I hope simple for me :)

It's possible to upgrade a WordPress.org-hosted plugin which was originally in a different folder than it's w.org slug too, so it's not a unique situation at all - it's one that is intended to be supported.

That seems like a similar use case. Did I miss an existing filter?

[dd32]

Did I miss an existing filter?

Nope, my point was that it's supposed to happen transparently, so that no filtering is needed.

I'll take a look into this today, I have a few ideas on how to approach it.

[afragen]

Replying to dd32:

Did I miss an existing filter?

Nope, my point was that it's supposed to happen transparently, so that no filtering is needed.

I'll take a look into this today, I have a few ideas on how to approach it.

It probably would happen transparently if I weren't renaming the destination directory.

Let me explain what happens. The plugin updates from GitHub source and downloads as user-repo-hash, instead of just repo. I use the upgrader_post_install filter to rename to repo but in shiny updates the referenced function gets the original destination and passes it. Because of this the new, updated plugin location isn't referenced and later in that function the get_plugins call is to the original location that doesn't exist.

[dd32]

Replying to afragen:

Let me explain what happens. The plugin updates from GitHub source and downloads as user-repo-hash, instead of just repo. I use the upgrader_post_install filter to rename to repo but in shiny updates the referenced function gets the original destination and passes it. Because of this the new, updated plugin location isn't referenced and later in that function the get_plugins call is to the original location that doesn't exist.

Ah. So the problem is that you're not updating the value of destination_name which is returned from the upgrader instance to what you're renamed it to - which is what the Ajax handler relies upon to correctly identify that the plugins folder has changed.

I'm specifically referring to this line which runs before that filter:

$this->result = compact( 'source', 'source_files', 'destination', 'destination_name', 'local_destination', 'remote_destination', 'clear_destination' );

I still don't think the filter you've proposed is correct, and that using the upgrader_post_install filter as you are doing so is also the incorrect thing to do. Kind of feels like upgrader_source_selection might be a better option to rename it on, you can probably just rename the source folder to what you want it to be and return that there.

[afragen]

Replying to dd32:

Replying to afragen:

Let me explain what happens. The plugin updates from GitHub source and downloads as user-repo-hash, instead of just repo. I use the upgrader_post_install filter to rename to repo but in shiny updates the referenced function gets the original destination and passes it. Because of this the new, updated plugin location isn't referenced and later in that function the get_plugins call is to the original location that doesn't exist.

Ah. So the problem is that you're not updating the value of destination_name which is returned from the upgrader instance to what you're renamed it to - which is what the Ajax handler relies upon to correctly identify that the plugins folder has changed.

But I am updating destination_name in upgrader_post_install.

I'm specifically referring to this line which runs before that filter:

$this->result = compact( 'source', 'source_files', 'destination', 'destination_name', 'local_destination', 'remote_destination', 'clear_destination' );

It seems that when AJAX updating occurs my code is firing and renaming the destinations. Somewhere the $upgrader->bulk_upgrade() doesn't have a post-processing filter.

I still don't think the filter you've proposed is correct, and that using the upgrader_post_install filter as you are doing so is also the incorrect thing to do. Kind of feels like upgrader_source_selection might be a better option to rename it on, you can probably just rename the source folder to what you want it to be and return that there.

To be clearer, my code already correctly renames the destination and the plugin update occurs correctly to the new destination even when using shiny updates. The issue is that in shiny updates the original destination is retrieved from wp_ajax_update_plugin() and I can't get the correct new version number.

FWIW, upgrader_post_install is much simpler to rename from than upgrader_source_selection based upon the parameters that are passed with each filter, at least for my purposes. ;)

[dd32]

But I am updating destination_name in upgrader_post_install.

But that information isn't passed back to the Upgrader instance, because there isn't a way to do so.

I'm thinking that the best option is to pass $this to the filters for context (or at least the upgrader_post_install filter) so that filters can update the results.

[afragen]

Replying to dd32:

But I am updating destination_name in upgrader_post_install.

But that information isn't passed back to the Upgrader instance, because there isn't a way to do so.

I'm thinking that the best option is to pass $this to the filters for context (or at least the upgrader_post_install filter) so that filters can update the results.

Would that mean that any changes that are done via the filter would be saved and show up correctly in this instance? If so, that should solve this issue.

[dd32]

Would that mean that any changes that are done via the filter would be saved and show up correctly in this instance? If so, that should solve this issue.

Yes, in your filter you'd do something like the following, which should just work..

function( $filter_args...., $upgrader_instance ) {
  // Perform rename
  $renamed_name = ...
  // Update the result of the upgrade routine
  $upgrader_instance->result['destination_name'] = $renamed_name;
}

[afragen]

As I trace the result through the function apply_filter gets the corrected values in arg[1] but returns $args[3] which has the original values.

So the filter never actually returns the corrected values. Bizarre.

[afragen]

Replying to dd32:

Would that mean that any changes that are done via the filter would be saved and show up correctly in this instance? If so, that should solve this issue.

Yes, in your filter you'd do something like the following, which should just work..

function( $filter_args...., $upgrader_instance ) {
  // Perform rename
  $renamed_name = ...
  // Update the result of the upgrade routine
  $upgrader_instance->result['destination_name'] = $renamed_name;
}

An $upgrader_instance isn't passed to this filter.

[dd32]

An $upgrader_instance isn't passed to this filter.

My proposal was to add it, however that can't be done until #27365 is resolved.

I still think your approach is wrong, I don't think there's really much that we should do to support it. Another way could be to extend Plugin_Upgrader and do it that way, but that'd also require you to jump through hoops.

WP_Upgrader really doesn't like packages that aren't in it's expected format..

[afragen]

Replying to dd32:

An $upgrader_instance isn't passed to this filter.

My proposal was to add it, however that can't be done until #27365 is resolved.

I still think your approach is wrong, I don't think there's really much that we should do to support it. Another way could be to extend Plugin_Upgrader and do it that way, but that'd also require you to jump through hoops.

WP_Upgrader really doesn't like packages that aren't in it's expected format..

I'm not clear on what the expected format is.

I guess what I expected in my approach was to use a filter that passed variables that allowed for an simpler renaming of the destination. Unfortunately, it appears that the upgrader_post_install filter really doesn't return anything. It returns an array or an error and it seems whatever the array is doesn't matter as nothing is really done with it.

Maybe I should be asking for an upgrader_modify_destination filter that fires just after the line

$this->result = compact( 'source', 'source_files', 'destination', 'destination_name', 'local_destination', 'remote_destination', 'clear_destination' );

something like

$this->result = apply_filters( 'upgrader_modify_destination', $this->result, $args['extra_hook'] );

[afragen]

Thanks for the input Dion.

[dd32]

I'm not clear on what the expected format is.

The expected format is a ZIP file similar to this:

my-plugin-name\
  - my-plugin-name.php

I'm assuming yours are in the following format, which is why it uses the zip name (user-project-hash) as the folder

my-plugin-name.php

[afragen]

Replying to dd32:

I'm not clear on what the expected format is.

The expected format is a ZIP file similar to this:

my-plugin-name\
  - my-plugin-name.php

I'm assuming yours are in the following format, which is why it uses the zip name (user-project-hash) as the folder

my-plugin-name.php

Actually my format is sort of correct. Downloads from GitHub, Bitbucket, and GitLab are in the format

username-my-plugin-name-hash/my-plugin-name.php

As you can see the directory needs to be renamed for consistent function.

[afragen]

Duplicate of #34661.

[dd32]

@afragen like previously suggested, you can use the upgrader_source_selection filter like so:

add_filter( 'upgrader_source_selection', '_upgrader_source_selection', 10, 3 );
function _upgrader_source_selection( $source, $remote_source, $upgrader_instance ) {
	global $wp_filesystem;

	$new_path = trailingslashit( $remote_source ) . basename( $source ) . '-dd32/';
	$wp_filesystem->move( $source, $new_path );

	return $new_path;
}

That suffixes all updates with -dd32 in the foldername, so you should be able to use the same thing.

[dd32]

#34661 was marked as a duplicate.

[dd32]

If that doesn't work, please just email me the code to dion at wordpress dot org and i'll make it work for you.

[afragen]

@dd32 thank you very much for your generous offer. I actually have working code for those filters.

Unfortunately, there is no code that can fix the issue in this ticket. It requires another filter that can adjust $this->result['destination_name'] and rather than limit a solution to just this; #34661 provides a more comprehensive solution.

[afragen]

@dd32 I've been thinking about this problem more and I may have another solution that doesn't involve adding a new filter.

I will send you an email about how to reproduce the current issue of shiny updates leaving an empty string for the version number. I don't think I can solve this issue without some help.

If you want my to explain how to test this issue here please let me know. It may take me a few days as I'm on call every other day through the next couple of weeks.

Thanks

[afragen]

@dd32 you are correct. Why did I even doubt. I've traced everything back through and upgrader_source_selection is the exact filter to use. However renaming would greatly benefit from the passing in the data in $args['hook_extra'].

Is there any way to do this?

Unfortunately just using the $upgrader instance allows for many permutations of Updaters, Bulk_Updaters, and updater skins to parse out the plugin or theme that is being updated. $args['hook_extra'] puts this info in a very simple place and no parsing of the user-repo-hash needs to be done. Because of the vast combinations of GitHub user names, repository names and hashes. The renaming code becomes ever so complex.

[afragen]

@dd32 I was able to re-write my code such that this ticket can now be closed. Thanks for all your direction and insistence that it really could be done using upgrader_source_selection.