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

Fixed issue when clicking the preview button from the node view full template would cause errors due to form token #27

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thiagocamposviana
Copy link
Contributor

Fixed issue when clicking the preview button from the node view full template would cause errors due to form token

…template would cause errors due to form token
@emodric
Copy link
Member

emodric commented Dec 21, 2018

Hm... Are you sure this is the right fix? What if the ezxformtoken extension is installed? As far as I know, that extension rewrites any POST submit to check for ezxform_token variable.

Also, I haven't ran into this issue before. Can you detail steps to reproduce this?

Thanks!

@thiagocamposviana
Copy link
Contributor Author

thiagocamposviana commented Dec 21, 2018

Tested this in ezp 2.3 after the changes submitted by Philipp to get back the "Preview" button at the top in the node view full view. If you click the button there is going to appear some errors related to the form token and also related to the language.

See:

bf0c1ca

Also:
48de585

and c9a4445

@emodric
Copy link
Member

emodric commented Jan 11, 2019

I still cannot reproduce this. Clicking the Preview button at the top of the full view takes me to the preview without issues.

@thiagocamposviana
Copy link
Contributor Author

@emodric What is the base ezp + legacy version you are using?

@emodric
Copy link
Member

emodric commented Jan 11, 2019

@thiagocamposviana eZ Platform 2.4.0 and eZ Publish Legacy 2018.09.2

@thiagocamposviana
Copy link
Contributor Author

Alright, I will try to reproduce and record this sometime next week and I will post the video here. I have just installed eZP 2.4 yesterday and I am sure the form token extension is checking for "_token" instead of "ezxform_token" with legacy bridge.

@emodric
Copy link
Member

emodric commented Jan 11, 2019

Alright, I've reproduced the issue, some caches were probably to blame :-o

Anyways, as far as I can see, there are multiple places where ezxform_token POST variable is being used in Netgen Admin UI, would you update all of them in this PR, so we can fix them all at once?

@thiagocamposviana
Copy link
Contributor Author

Looks like a good idea.

@andrerom
Copy link

Maybe we should make this configurable and let legacy bridge set ezxformtoken to use the Symfony convention? That is the approach used for more other things around tis.

@emodric
Copy link
Member

emodric commented Mar 22, 2019

@andrerom For Netgen Admin UI, it doesn't need to be configurable, I think, since it cannot be used in pure legacy context. But as an idea it's good for ezpublish-legacy with legacy bridge.

@andrerom
Copy link

andrerom commented Mar 22, 2019

But as an idea it's good for ezpublish-legacy with legacy bridge.

This is the context I'm talking about, but yes should have made the comment on the other PR and pinged you ;)

@emodric
Copy link
Member

emodric commented Mar 22, 2019

What do you think @thiagocamposviana ?

@thiagocamposviana
Copy link
Contributor Author

It should be an enhancement, as of right now it as long this is a bug, the present solution solves the bug without allowing any configuration, which is ok to certain extent, the enhancement would be good as a future step but not mandatory atm as long it might take way longer to review and approve and we require a solution for the bug itself not necessary containing the enhancement.

@andrerom
Copy link

True, but this one:
ezsystems/ezpublish-legacy#1420

is not clean.

So you kind of need the injection => to handle this cleanly => to be able to get this merged => in order to solve the bug.

Right? ;)

@thiagocamposviana
Copy link
Contributor Author

thiagocamposviana commented Apr 2, 2019

Alright, as long that affects only a php file I think I can make the token identifier configurable easily.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants