Problem/Motivation

The methods FileItem::generateSampleValue() and ImageItem::generateSampleValue()
create URIs with unnecessary slashes if the file_directory setting is not configured.

For example:
I'm using devel_generate to generate nodes which have file attachments.
Drupal\file\Plugin\Field\FieldType\FileItem::generateSampleValue()
generates uris with 3 slashes when files are not configured to be in a subdirectory. so if $settings['file_directory'] is configured it's great
private://my_dir/blah.txt
otherwise
private:///blah.txt

Steps to reproduce

From the description
1. Use devel_generate
2. generate some content for something that has file attached
3. Don't set file_directory in settings.php

Proposed resolution

Suggested fix. Instead of
$destination = $dirname . '/' . $random->name(10, TRUE) . '.txt';
this

Instead of updating doGetUploadLocation() to add a trailing slash (which could break existing usage), we now add the trailing slash inline within the same function where it's needed:

FileItem 

$dirname .= str_ends_with($dirname, '/') ? '' : '/';
$destination = $dirname . $random->name(10, TRUE) . '.txt';

ImageItem

destination_dir .= str_ends_with($destination_dir, '/') ? '' : '/';
$destination = $destination_dir . basename($path);

Remaining tasks

Review

User interface changes

N/A

Introduced terminology

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

N/A

Issue fork drupal-3471194

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

matslats created an issue. See original summary.

quietone’s picture

quietone commented
Version: 10.3.x-dev » 11.x-dev

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.

kim.pepper opened merge request !10316

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Active » Needs review
smustgrave’s picture

smustgrave commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

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

quietone’s picture

quietone commented
Status: Reviewed & tested by the community » Needs work

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

Anonymous’s picture

Anonymous (not verified) commented

akulsaxena made their first commit to this issue’s fork.

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs work » Needs review

@quietone
I applied the suggested changes
Please take a look

Anonymous’s picture

Anonymous (not verified) commented

Ran tests and the Pipeline is green

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Left comments on MR.

Anonymous’s picture

Anonymous (not verified) commented

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

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Feedback appears to be addressed. Thanks

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Needs work

Left some questions on the MR

shalini_jha made their first commit to this issue’s fork.

shalini_jha’s picture

shalini_jha commented

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.

shalini_jha’s picture

shalini_jha commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Responded to open thread.

shalini_jha’s picture

shalini_jha commented
Assigned: Unassigned » shalini_jha
shalini_jha’s picture

shalini_jha commented
Assigned: shalini_jha » Unassigned
Status: Needs work » Needs review

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.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

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

shalini_jha’s picture

shalini_jha commented
Assigned: Unassigned » shalini_jha
shalini_jha’s picture

shalini_jha commented
Issue summary: View changes
shalini_jha’s picture

shalini_jha commented
Issue summary: View changes
shalini_jha’s picture

shalini_jha commented
shalini_jha’s picture

shalini_jha commented
Title: File::generateSampleValue makes bad Uris » ImageItem and FileItem generateSampleValue methods makes bad Uris
shalini_jha’s picture

shalini_jha commented
Assigned: shalini_jha » Unassigned
Status: Needs work » Needs review

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.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Thanks for following this one through

quietone’s picture

quietone commented
Status: Reviewed & tested by the community » Needs work

Yes, thanks @shalini_jha,

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

shalini_jha’s picture

shalini_jha commented

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

shalini_jha’s picture

shalini_jha commented
Assigned: Unassigned » shalini_jha

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

shalini_jha’s picture

shalini_jha commented
Assigned: shalini_jha » Unassigned
Status: Needs work » Needs review

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.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Believe feedback has been addressed here.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs review

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?

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Sounds like NW for more workshop.

shalini_jha’s picture

shalini_jha commented
Issue summary: View changes
Status: Needs work » Needs review

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.
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

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Needs review » Needs work

Left some suggestions.

shalini_jha’s picture

shalini_jha commented

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

shalini_jha’s picture

shalini_jha commented
Status: Needs work » Needs review

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. Thank you

shalini_jha’s picture

shalini_jha commented
Issue summary: View changes
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Believe feedback has been addressed

alexpott made their first commit to this issue’s fork.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

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

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 11.x-dev » 10.5.x-dev
Status: Reviewed & tested by the community » Fixed

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!

  • alexpott committed ff046030 on 10.5.x 
    Issue #3471194 by shalini_jha, kim.pepper, smustgrave, quietone,...

  • alexpott committed 3f31fab7 on 10.6.x 
    Issue #3471194 by shalini_jha, kim.pepper, smustgrave, quietone,...

  • alexpott committed 08080669 on 11.2.x 
    Issue #3471194 by shalini_jha, kim.pepper, smustgrave, quietone,...

  • alexpott committed ec48de10 on 11.x 
    Issue #3471194 by shalini_jha, kim.pepper, smustgrave, quietone,...

Status: Fixed » Closed (fixed)

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