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.