Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TASK: Refactor to fusion #26

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

mficzel
Copy link
Member

@mficzel mficzel commented Dec 6, 2019

Replace the Fluid rendering and Plugin logic with pure AFX and Fusion based rendering.

The rendering is very basic and non opinionated but can be easyly adjusted by overriding
the rendering prototypes:

prototype(Flowpack.Neos.FrontendLogin:LoginForm) {
    loginPrototype = 'Vendor.Site:LoginForm.Login'
    logoutPrototype = 'Vendor.Site:LoginForm.Logout'
}

@mficzel
Copy link
Member Author

mficzel commented Dec 6, 2019

This is an enhanced version of #25

@mficzel
Copy link
Member Author

mficzel commented Dec 6, 2019

An important question for the review is wether passing the redirect target uris to the controller is safe or wether we have to add some signing here.

Copy link
Member

@markusguenther markusguenther left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks awesome and except an tiny detail it also works like charm.
When I setup an page like the member area after login as redirect it does not work.

But if I am choosing a non protected page it works fine.
Did not see the issue yet inside the code.

element {
redirectAfterLoginUri = Neos.Neos:NodeUri {
node = ${q(node).property('redirectAfterLogin')}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not work when the target page needs the front-end user privilege.
In this case a url to the login page is created instead.

So I think we have to access the node in the authentication controller somehow.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, maybe getting rid of the plugin has some side effects :-/ not sure how to solve this safe and sane.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A way around would be to drop the redirectAfter-feature and just always send the user back to the referer wich uses arguments that are signed with an hmac.

However finding a way to transport this safely to the controller without relying on plugins would be a really nice example for such cases.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder a bit how it worked before. Did that work before?

Other ways:

  • Providing a linkeditor for both uris would of course also make it possible to give a relative url. Not the best solution but maybe would anyway make sense for other usecases.
  • Give the login form node identifier to the controller and let it build the uri from its properties when it has the required privileges. Would require us to duplicate some code from the NodeUri builder I guess.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The plugin evaluates the path redirectAfterLoginUri after the login and not immediately. Maybe we could create a lazyEvaluationProxy fusionObject as wrapper for the NodeUri :-/

BTW: I just found out that the FusionTemplate does similar things for all untyped properties.

Copy link
Member Author

@mficzel mficzel Dec 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Core of the problem is that props are usually evaluated immediately and not once they are accessed. Changing that is not that simple as you can see here neos/neos-development-collection#2738

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So which way should we go then. Back to the plugin for now?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either use your original pr or find a solution here which will take time and a sprint would be a good idea.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But then I would prefer the plugin approach for now. Would love to have this and not wait 5-6 months.
With some more experience from this we also have a better idea until the sprint to remove the plugin part.

Do you want to create another PR with my commit + your cleanup adjustments?


<div>
<label for="flowpack-neos-frontendlogin-username" class="col-sm-2 control-label">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<label for="flowpack-neos-frontendlogin-username" class="col-sm-2 control-label">
<label for="flowpack-neos-frontendlogin-username">

</div>
<div>
<label for="flowpack-neos-frontendlogin-password" class="col-sm-2 control-label">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<label for="flowpack-neos-frontendlogin-password" class="col-sm-2 control-label">
<label for="flowpack-neos-frontendlogin-password">

form.target.controller="Authentication"
form.target.action="logout"
attributes.class="form-horizontal clearfix"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
attributes.class="form-horizontal clearfix"

@Sebobo
Copy link
Member

Sebobo commented Dec 7, 2019

Thanks for making my PR so much better :)

@mficzel
Copy link
Member Author

mficzel commented Dec 7, 2019

@Sebobo did not dare to push your branch as those changes are quite radical and we still might run into things that require a plugin.

composer.json Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants