- 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. - ๐บ๐ธUnited States smustgrave
Feedback appears to be addressed. Thanks
- ๐ฆ๐บAustralia larowlan ๐ฆ๐บ๐.au GMT+10
Left some questions on the MR
- 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
2 days 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.