search

contao / core-bundle

[READ-ONLY] Contao Core Bundle
GNU Lesser General Public License v3.0
123 stars 58 forks source link

[WIP] feature ce access #1560

Closed m-vo closed 4 years ago

m-vo commented 6 years ago

As discussed in Mumble on July 5th, we want to merge the existing legacy code.

Rewrite to use existing legacy code / you'll find the old PR description in the edit history.

Todo:

Things to fix/rework later on:

Notes:

I adapted the function names from the ce_access extension but I don't really like them. Now there is a button callback for the operations edit, copy, cut named hideButton(), an existing one for delete and toggle named deleteElement() and toggleIcon(). IMO all of them should rather be called something like getButtonMarkup() because that's what they return. Also, there is logic incorporated in toggleIcon() could be moved to another load callback. It's legacy code and we cannot change names (and probably don't want to move code I guess)...

Toflar commented 6 years ago

Thank you for the PR @m-vo! I'm not sure I like the concept though 😄 I mean, it's perfect that you started separating UI from permission logic but it doesn't really look 100% separated. I don't understand why Access would do public function filterContentElements(DataContainer $dc). That's already 2 responsibilities:

To me, the onload_callbacks should go to some EventListener class that does the DCA stuff. And for the permissions we should use Security Voters.

I guess in your draft Access becomes ContentElementEventListener. The idea behind separating it even more is that we need to be able to ask the permission system from elsewhere, without even having a request. This is a fundamental requirement for stuff like a general API etc. So if you want to refactor permissions, we should use voters. But it's going to be a lot of work 😄

m-vo commented 6 years ago

@Toflar I totally agree on what you are saying. My first goal was to get all those button callbacks out and set up a general structure. The access related parts (esp. the filtering) needs more refactoring before I can separate responsibilities. There are several return points that affect logic for instance.

To me, the onload_callbacks should go to some EventListener class that does the DCA stuff.

That was also on my mind. I hadn't had the time to analyze the flow, yet. The toggle visibilty stuff directly issues redirects (should maybe be redirect exceptions) may throw an AccessDeniedExeption and triggers callbacks. So we have to be very careful not to break something.

And for the permissions we should use Security Voters.

Yes, but that's something that needs some discussion first and should be separated from this PR. If we agree on a concept I can work on it or at least how the interfacing parts from this PR should look like.

Toflar commented 6 years ago

I agree. But you introduce a new service which may be used by others. That's not a good idea if it's not the final concept 😄

m-vo commented 6 years ago

Sure, we could make things private though were possible. And I'm happy to change things, as it's WIP atm. 😄

leofeyer commented 6 years ago

As I have told you in Salzburg, it is generally not a good idea to introduce new concepts without discussing them with the core team in advance. Chances are that you either have to rework everything or that the PR is closed unmerged (see #355).

m-vo commented 6 years ago

The only 'concept part' right now is to use services for the callbacks - isn't that considered a best practice for extensions anyways? But yeah, like said before, I have no problem changing this PR, so it basically adds and extends around the old tl_content class functions. I just don't believe it's the right thing to do as IMHO it makes things even harder to pull apart one day + there won't be a way to add tests (but that's arguable of course).

m-vo commented 6 years ago

So, do you want me to rewrite this in 'old code style' (which would mean new functions in tl_content class and less abstraction) or shall I continue refactoring?

leofeyer commented 6 years ago

As discussed in Mumble on July 5th, we want to merge the existing legacy code.

m-vo commented 6 years ago

@aschempp How should the core-bundle deal with the existing ce_access extension? There was a brief discussion about this on the mumble call but I don't recall the conclusion. I'm not sure if we want to replace or provide it in our composer.json and in which version? Also: do we need to specify conflicts?

My understanding: If using replace we must specify an API compliant version. But there isn't one as the CeAccess class will be missing for example. So if any extension builds on top of the package, it possibly won't be compatible anymore with the merged version. But ce_access could also not be installed in parallel anymore as for example database field names would conflict.

I guess the merged changes from the ce_access develop branch would otherwise have been released as 3.0.0 as there are changes that need migration. But replacing 3.0.0 and adding a conflict for < 3.0.0 seems a bit odd and would need everyone to adapt their requirements, so we could as well just add a conflict. The install tool would propose dropping the elements column if the package is removed but 4.6 not yet installed without the user knowing that it would be used/migrated further on.

On top, the ce_access package has requirements for contao-community-alliance/composer-plugin which the core doesn't have (and want) that will get dropped when replacing.

I'm confused... :confused:

aschempp commented 6 years ago

I'm not exactly sure how composer behaves in such cases. Ideally, the core should provide terminal42/contao-ce-access, and therefore if you require it in your root composer.json the extension should simply not be installed. If provide does not work, we can simply add a conflict which prevents any update if ce-access is installed.

Be aware that dev-develop represents version 2.0. As it is still develop, you cannot consider the API stable, so developers can only rely on a CeAccess class if the used the stable 1.x version.

Toflar commented 6 years ago

„provide“ is for virtual packages. I think „replace“ would be correct here but needs to be tested.

m-vo commented 6 years ago

Be aware that dev-develop represents version 2.0.

You mean allthough it has features and possibly breaking changes in it (and therefore would be released as 2.1.0 or 3.0.0) it's considered a 2.0.* version until tagged otherwise?

[...] , so developers can only rely on a CeAccess class if the used the stable 1.x version

Isn't the current stable version 2.0.5? But even then if we don't supply the class (what we're not doing), this is still a break when replacing the package?

„provide“ is for virtual packages. I think „replace“ would be correct here but needs to be tested.

It think so, too. That's what the composer docs says:

This is mostly useful for common interfaces. A package could depend on some virtual logger package, any library that implements this logger interface would simply list it in provide.

So something like this, then?

    "replace": {
        "terminal42/contao-ce-access": "<2.1"
    }
aschempp commented 6 years ago

You mean allthough it has features and possibly breaking changes in it (and therefore would be released as 2.1.0 or 3.0.0) it's considered a 2.0.* version until tagged otherwise?

Sorry I was wrong. It's obviously version 3.0, I thought we didn't have a 2.0 yet.

So something like this, then?

We would replace any version of ce-access, not just <2.1

m-vo commented 6 years ago

Allright.

We would replace any version of ce-access

Ok I guess this means the extension will be discontinued as soon as we are merging it into the contao core (besides Contao < 4.6 and changes that do not alter the database structure).

aschempp commented 6 years ago

Ok I guess this means the extension will be discontinued as soon as we are merging it into the contao core (besides Contao < 4.6 and changes that do not alter the database structure).

Absolutely

leofeyer commented 4 years ago

See https://github.com/contao/contao/pull/705