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 EZP-22823: exception in Preview w/ website toolbar #83

Closed
wants to merge 1 commit into from

Conversation

4 participants
@bdunogier
Copy link
Member

commented May 9, 2014

Fixes http://jira.ez.no/browse/EZP-22823

An exception is thrown by Twig/Stash if the websitetoolbar is enabled with the provided configuration, and a Preview is requested.

In the PreviewController, a fake Location is created, with among others idset to null. This null ID is passed on to the website toolbar render, and content is loaded by the API from a null ID.

If we go this way, it also needs to be documented like this.

@pspanja

This comment has been minimized.

Copy link
Contributor

commented May 9, 2014

+1

1 similar comment
@andrerom

This comment has been minimized.

Copy link
Member

commented May 9, 2014

+1

@bdunogier

This comment has been minimized.

Copy link
Member Author

commented May 9, 2014

I'm actually -1 on this one, I'd rather go with the one in ezsystems/ezpublish-kernel#845

@pspanja

This comment has been minimized.

Copy link
Contributor

commented May 9, 2014

@bdunogier It seems there is no reason not to merge both?

@andrerom

This comment has been minimized.

Copy link
Member

commented May 9, 2014

is there any issues with merging this? this avoid the sub request if location is missing basically, but will it cause cache issues or something?

@bdunogier

This comment has been minimized.

Copy link
Member Author

commented May 9, 2014

No, @pspanja, no reason whatsoever.

This way is technically more correct, but I don't think we can safely expect users to always pick the right piece of code. We can do the check ourselves, and it's fine this way.

@bdunogier bdunogier closed this May 9, 2014

@lolautruche

This comment has been minimized.

Copy link
Contributor

commented May 9, 2014

ezsystems/ezpublish-kernel#845 is indeed far better.

@bdunogier bdunogier deleted the EZP-22823-preview_wt_exception branch May 9, 2014

@andrerom

This comment has been minimized.

Copy link
Member

commented May 9, 2014

@pspanja Discussed this with @bdunogier and I suggested if we want to fix this in template in a forward compatible and clean way we should add parts of the API value object changes suggested for Location draft, but not add service api's and implementation for it yet. This would allow preview helper to specify the fake Location returned in ezsystems/ezpublish-kernel@73a7e97#commitcomment-6268391 to be a draft, so that the check here can check if location is draft or not instead of having a "hack" checking if id is null.

Basically this change: ezsystems/ezpublish-kernel@6e79bd7 without the ws issue.

What do you think?

(but for better template use we should consider also adding a method like isDraft() so template does not have to hardcode status values)

@pspanja

This comment has been minimized.

Copy link
Contributor

commented May 9, 2014

@andrerom ok, that seems to be worthwhile.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.