[RFC] File accessibility check for various content elements

[m-vo]

as requested in #1531

Checks are now made on load of text, accordionSingle, hyperlink, image (single source) as well as gallery and player (multi source) content elements: a message gets show if any of the selected sources is protected.

Open questions:

• Should we rather target all content elements and explicitly remove download / downloads instead so that it also applies to extensions that are using these fields?
• Or: Which core extension content elements should be added?
• The grammar for selecting one protected element in the multi source case is a bit off but I didn't want to add even more language strings - any better solution?

Remarks:

• The functionality for isProtectedPath() is identical to this function inside DC_Folder https://github.com/contao/core-bundle/blob/ac908c6f16bc1cd110569be8661043cbf02d74ae/src/Resources/contao/drivers/DC_Folder.php#L2984-L3005 It wouldn't make any sense to make the DC_Folder function public/static (wrong responsibility) so this should probably be factored out some day (DRY).

[discordier]

Pretty neat, however:

1. I'd favor a generic solution (refactor the callback out into a helper class and allow to call it from other DCAs as well).
2. I'd rephrase the message added as to include the file names. The information that 3 files are protected does not help much, as I do not know which of the 4 cats are now protected and which is not.

[m-vo]

I'd favor a generic solution (refactor the callback out into a helper class and allow to call it from other DCAs as well).

I had that in mind as well. Wdyt core devs?

I'd rephrase the message added as to include the file names. The information that 3 files are protected does not help much, as I do not know which of the 4 cats are now protected and which is not.

Good point. But perhaps we should only show the - say - first 5 to keep the message a reasonable size? You have selected 34 files/folders which aren't public and possibly not accessible in the frontend: a/b/first.jpg, a/b/second.jpg, a/b/third.jpg, x/abc.png, x/def.png, …

[Toflar]

Nice helper! However, it shouldn't go to the DataContainer or any callback at all. It should go to the widget itself because the message should also be displayed if the widget is used standalone (outside of any DCA when directly instantiated). The message makes absolutely no sense on top. You wouldn't even see it if the edit mask was very long and you were editing something further down 😄

[m-vo]

It should go to the widget itself because the message should also be displayed if the widget is used standalone

Imo we would need an option to enable/disable it then. Because the check should only apply to files that are linked to in the frontend.

(In case of the image reordering widget best UI would probably be to actually mark the affected images directly - e.g. with a small lock symbol, greyed out, whatever..)

[Toflar]

Exactly. It's widget configuration which is why the widget should handle it 😄

[m-vo]

Besides this: Do we want to add something like isProtected() to the files/folder class? Or which bit of the filesystem infrastructure is responsible for knowing this?

[Toflar]

Might be a good idea, right now it's in DC_Folder which is a bit strange. The Folder already has protect() and unprotect() methods.

[m-vo]

Yep, Folder and probably File as well to avoid boilerplate code (it's already coupled to Folder, so implementation could be in there). Maybe a second PR for that?

[Toflar]

Yeah, try to keep the PR's simple and small. Chances are way higher to get them merged then 😄

[m-vo]

Yeah, try to keep the PR's simple and small. Chances are way higher to get them merged then smile

:+1:

The API would behave a bit strange so. Example:

$folder->isProtected(); // false, because parent has .public file in it
$folder->protect(); // does nothing because there is no .public file
$folder->isProtected(); // still false

But that's a problem of the protect() method which should probably refuse doing so if a parent folder is public. + The API has inverse logic for historic reasons I suppose.

Would you rather see isProtected() to comply with the current API or isPublic() to represent the actual structure?

[m-vo]

Just for the records: I'm waiting for the outcome of #1601 till I continue with this PR.

[leofeyer]

We have discussed #1601 in Mumble today.

[leofeyer]

I'm closing this in favor of contao/contao#168.