search

silverstripe / silverstripe-assets

Silverstripe Assets component
BSD 3-Clause "New" or "Revised" License
9 stars 65 forks source link

AssetAdapter - If the visibility is already correct, do not try to re-set it. #563

Closed nathanbrauer closed 8 months ago

nathanbrauer commented 11 months ago

AssetAdapter.php - If the visibility is already correct, do not try to re-set it.

Attempting to set the visibility of a file/folder when the filesystem is already set correctly causes "Operation not permitted" errors on some Linux servers in /dev/build. This often leads to a fatal error preventing the completion of the /dev/build.

Issue

nathanbrauer commented 11 months ago

This fix is for inclusion in the next minor release of silverstripe/silverstripe-framework:^4.x which uses silverstripe/silverstripe-assets:^1.x

However, I've already checked and this commit is also safe to rebase/cherry-pick/whatever into silverstripe/silverstripe-assets:^2.x as well.

GuySartorelli commented 11 months ago

Hi, Thanks for this. Please retarget this to the 1.13 branch so that it can be released as a patch.

nathanbrauer commented 11 months ago

My pleasure! Let me know if I did that right! :)

GuySartorelli commented 11 months ago

Yes, looks like you have, thank you! You'll notice I've created an issue that I've associated with this PR - that will let us track this on our internal board. It's in our backlog to be properly reviewed when we have capacity to do so.

emteknetnz commented 10 months ago

Code looks good, I'm running an extra test of this PR on running against the silverstripe/installer CI - if that goes green I'm happy to merge https://github.com/emteknetnz/silverstripe-installer/actions/runs/5947561612

Update:

This phpunit test failure is existing

However this behat test failure is not

Saving HTML into /home/runner/work/silverstripe-installer/silverstripe-installer/artifacts/screenshots/zz_insert-an-image.feature_122.html And the "Content" HTML field should contain "My alt updated & saved" # SilverStripe\Framework\Tests\Behaviour\CmsFormsContext::theHtmlFieldShouldContain() The string "My alt updated & saved" should be found in the HTML of the element matching name "Content". Actual content: "<p>[image src="/assets/folder1/3d0ef6ec37/file1.jpg" id="2" width="50" height="50" class="leftAlone ss-htmleditorfield-file image" title="My title text updated &amp;amp;amp;amp; saved" alt="My alt updated &amp;amp;amp;amp; saved"]My awesome content</p>" (Behat\Mink\Exception\ElementHtmlException)

Issues appears to be & getting turned into &amp;amp;amp;amp;, presumably by multiple saves?

I've replicated the behat failure locally and have confirmed that it does not happen with the most recent version of silverstripe/assets - 53e8331. I tried rebase the PR branch on top of 53e8331 on my local however that didn't resolve the behat failure.

emteknetnz commented 10 months ago

Updated installer test using pull-request - if this goes green then I'm happy with this pull-request https://github.com/emteknetnz/silverstripe-installer/actions/runs/6116321876

emteknetnz commented 9 months ago

@nathanbrauer Have you had a chance to look at this one. I'll look to close this PR in a week or so if there's no further activity on it

nathanbrauer commented 9 months ago

It's on my radar and I'll come back to it soon (currently working through some other priorities).

emteknetnz commented 8 months ago

Looks like there hasn't been any work done on this PR for a while. I'll close this for now. Feel free to reopen this if further work is done on it.