File::generateSampleValue makes bad Uris

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to our policies.

kim.pepper → made their first commit to this issue’s fork.

Cleaned up the issue summary some.

I see test-only has already been ran https://git.drupalcode.org/issue/drupal-3471194/-/jobs/3468388

I didn't see any existing file that the test coverage could be moved to so standalone file seem fine.

LGTM

I made some suggestions in the MR for coding standards and to add some comments.

@quietone
I applied the suggested changes
Please take a look

Ran tests and the Pipeline is green

Left comments on MR.

Hi @smustgrave
I made the suggested changes
Please give it a look.

Feedback appears to be addressed. Thanks

Left some questions on the MR

I have replicated the issue and reviewed the feedback on the MR, which makes sense. I have added the comments based on my findings. Additionally, I noticed that the assertion was passing every time, so I updated it and tested the change. Moving this to NR for confirmation or any further guidance.

Responded to open thread.

Thank you for the review.
I have implemented the changes in the doGetUploadLocation method and reviewed all occurrences of this method. While we already addressed the issue in the FileItem class, I found that the ImageItem class was handling file paths in a similar way appending static slash, so I have applied the same fix there as well. To ensure the changes work as expected, I have added test coverage for scenarios involving a blank file directory in the ImageItem class.
Please review the changes and let me know if any further adjustments are needed.

Last thing but if we are expanding scope believe title/summary need to reflect please

Thank you for the review. You are right; I have updated the issue summary and title to reflect the changes we have made so far.

Thanks for following this one through

Yes, thanks @shalini_jha,

On a second look here I found a few things, see the comment in the MR.

Thank you for the review @quietone, I will look into the feedback.

Assigning this me for sometime , as i am working on it now to complete the feedback.

I have addressed all the feedback and made some improvements. I refactored the code to create a more generic method for the test. Additionally, I updated the comments, After re-running the tests, I verified that they are working correctly for both FileItem and ImageItem, handling both empty and custom file directories.
I am moving this to NR. Kindly review and let me know if any further updates are required.

Believe feedback has been addressed here.

So if I understand the MR correctly this changes doGetUploadLocation() so it now always includes a trailing slash.

Unfortunately this is a public API change:

  public function getUploadLocation($data = []) {
    return static::doGetUploadLocation($this->getSettings(), $data);
  }

Existing callers might not expect the trailing slash, so this could break other usages of this method in contrib or custom code?

Is there another way of fixing this without changing this method's return value?

Sounds like NW for more workshop.

Thanks for the feedback!
Based on your suggestion, I haven’t modified the doGetUploadLocation() method. Instead, I added a trailing slash check directly in the same function where the destination path is being built for both FileItem and ImageItem. This way, we avoid changing the method’s return value and reduce the risk of breaking other usages.

Could you please review and let me know if this approach works, or if further changes are needed?
I have also update the IS based on my current approach for more clarity. pipeline is green so moving this for your review.

Kindly review, Thanks

Left some suggestions.

Thank you for the review and feedback. i am working on it.

Thank you for your feedback!
I’ve addressed all the points as suggested, and rerun the pipeline — it's now passing.

I’ve also updated the issue summary to reflect the changes made based on the MR feedback.
Please have a look and let me know if anything further needs to be adjusted.

Believe feedback has been addressed

Whoops we cannot use rtrim here because of public:// - nice tests! Reverted my noise. Sorry.

Backporting to 11.2.x,10.6.x and 10.5.x as a bugfix.

Committed and pushed ec48de10e88 to 11.x and 0808066939c to 11.2.x and 3f31fab7c51 to 10.6.x and ff046030205 to 10.5.x. Thanks!

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