- Issue created by @matslats
- 🇳🇿New Zealand quietone
Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to our policies.
- 🇦🇺Australia kim.pepper 🏄♂️🇦🇺Sydney, Australia
kim.pepper → made their first commit to this issue’s fork.
- Merge request !10316[#3471194] Remove extra slash from generate sample value → (Open) created by kim.pepper
- 🇺🇸United States smustgrave
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
- 🇳🇿New Zealand quietone
I made some suggestions in the MR for coding standards and to add some comments.
- First commit to issue fork.
- 🇮🇳India akulsaxena
@quietone
I applied the suggested changes
Please take a look - 🇮🇳India akulsaxena
Hi @smustgrave
I made the suggested changes
Please give it a look. - First commit to issue fork.
- 🇮🇳India shalini_jha
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.
- 🇮🇳India shalini_jha
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. - 🇺🇸United States smustgrave
Last thing but if we are expanding scope believe title/summary need to reflect please
- 🇮🇳India shalini_jha
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.
- Status changed to Needs work
3 months ago 2:59am 27 January 2025 - 🇳🇿New Zealand quietone
Yes, thanks @shalini_jha,
On a second look here I found a few things, see the comment in the MR.
- 🇮🇳India shalini_jha
Thank you for the review @quietone, I will look into the feedback.
- 🇮🇳India shalini_jha
Assigning this me for sometime , as i am working on it now to complete the feedback.
- 🇮🇳India shalini_jha
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. - 🇬🇧United Kingdom longwave UK
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?
- Status changed to Needs review
2 days ago 5:46am 24 April 2025 - 🇮🇳India shalini_jha
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
- 🇮🇳India shalini_jha
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.