I think we need to know the reasoning why subdir set to empty, and concatenated to the end of path.

Adding it in here, could lead to an incorrect path if a later function looks at both path and subdir.

e.g. uploads/2024/02/2024/02.

I'm not familiar with this codebase, but that's my first cautious impression.

With respect, subdir is set empty because the recent Pull Request set it empty.

Clearly the combination of base WordPress and the existing plugin does not need the component subdir as it works without it. It only needs path and basedir.

Seemingly it is the further processing by

Now the source of the values (input to the filter) ultimately comes from _wp_upload_dir. This contains the code:
` $basedir = $dir;
$baseurl = $url;

$subdir = '';
if ( get_option( 'uploads_use_yearmonth_folders' ) ) {
	// Generate the yearly and monthly directories.
	if ( ! $time ) {
		$time = current_time( 'mysql' );
	}
	$y      = substr( $time, 0, 4 );
	$m      = substr( $time, 5, 2 );
	$subdir = "/$y/$m";
}

$dir .= $subdir;
$url .= $subdir;

`
where $dir is the standard upload directory. So it gets the Uploads directory, finds out the subdir part and adds it to dir and url.

So my assumption is that if I'm going to modify the array, then I need to make it consistent with whatever standard WordPress does.

And that means that I need to set the output value of subdir to be the same as what was input to the filter - which is what my change does.

If there is further concatenation that creates the issue above, then it will also do it when this plugin is not active.

FWIW. If sub-directories are used then with my change I expect the standard logic of wp-amazon-s3-and-cloudfront to work fine where you have proposed a change to it, Leaving it as empty will fail.

With respect, subdir is set empty because the recent Pull Request set it empty.

My question was, why was it not set in the first place? And why is it concatenated to the end of path?

Apologies for nit finishing the sentence:
Seemingly it is the further processing by

It was to be:
Seemingly it is the further processing by the S3 plugin that is having an issue.

@benbalter
Thanks for approving the request.

@earthlingdavey
Whilst I am convinced that this corrects the logic, there might be additional issues with this plugin.
I have not had time to delve into the processing. Why may there be issues?
Because, if you have used a different directory from the standard one, then WPDR changes the upload directory from the standard one to the document library when it wants to serve the file.
If the S3 does this in an offline mode, say, then the replacement process may not occur properly.

Put simply, if you want to move the documents into S3 (or another remote store), then I would recommend that you initially load them into the standard upload directory.

I see. Obviously we have different POV. You have significant experience with this plugin. Which I'm grateful for BTW :) And I, on the other hand don't have so much.

I would be super cautious about changing too much on a plugin that was initially written by someone else over a decade ago. But, that's just me.