- 🇺🇸United States smustgrave
This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.
This could use an issue summary update for the proposed solution. Seems there is still discussion going on so when the path forward is decided it should be added to the IS. Will help the committer.
- Merge request !3468Issue #2704931: Provide Drupal root path on file system admin form → (Open) created by voleger
- Status changed to Needs review
over 1 year ago 10:41am 14 February 2023 - Status changed to Needs work
over 1 year ago 5:19pm 14 February 2023 - 🇺🇸United States smustgrave
Thanks!
Code wise I think it looks fine. Not sure if we will need a trigger_error for the new parameter or not.
This could use a simple change record though to announce the new parameter option.
- Status changed to Needs review
over 1 year ago 6:38pm 14 February 2023 - 🇺🇦Ukraine voleger Ukraine, Rivne
Added CR https://www.drupal.org/node/3341696 →
Updated constructor to handle new parameter addition. - Status changed to RTBC
over 1 year ago 1:42am 15 February 2023 - 🇺🇸United States smustgrave
Thanks think this good for committer review now.
- Status changed to Needs work
over 1 year ago 7:02am 18 February 2023 - 🇳🇿New Zealand quietone New Zealand
This is changing the user interface, adding tag. As such this should have before and after screenshot available from the issue summary. This is also adding text to the public and private file descriptions, so I think this should have a usability review.
I read the change record, well several times, and I am still not sure what 'related' means in "public, private, and temporary paths are related". Tagging for a change record update. This is not a complicated change but a screenshot could be useful in the change record.
I skimmed the patch and made some comments. I also note this was set to RTBC with an unresolved comment.
- Status changed to Needs review
over 1 year ago 11:43am 21 February 2023 - 🇺🇦Ukraine voleger Ukraine, Rivne
Changed the relative paths with absolute as this will show the actual location of specific stream wrappers.
Added check for path accessibility. The realpath() file_system method fails absolute path resolving if the directory not exists.
I have added screenshots before → , after → , and after with warning messages → . - Status changed to Needs work
about 1 year ago 11:52pm 15 April 2023 - 🇺🇸United States smustgrave
Brought this up in #ux channel and it was briefly looked at but was mentioned
taken a look but it would be helpful if the issue summary would have been updated illustrating the changes that will be made. at the moment the ui changes section only states The content of the page File system will be updated. . even if i apply a patch i consider it helpful to get an overview which changes were made.
- 🇺🇦Ukraine voleger Ukraine, Rivne
I agree that IS needs to be updated, but please check the screenshots from #35.
The latest changes in MR convert paths to absolute if they are relative. And if the destination does not exist, the path value is printed as is (relative or absolute) with an appropriate warning message that reuses existing sentences and translations.
In case such behavior is acceptable I'll update IS as I can. - 🇺🇦Ukraine voleger Ukraine, Rivne
rerolled
assets filesystem wrapper added since then
if #35 comparison approach is good to go with, I'm ready to create updated screenshots and attach them to the IS.