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

BUGFIX: Change return types of some Fusion methods to mixed #2762

Merged
merged 1 commit into from Nov 24, 2019

Conversation

ComiR
Copy link
Contributor

@ComiR ComiR commented Nov 5, 2019

ba8d141 introduced the return type string for Fusions RenderViewHelper, so we must always return a string (not null).

Copy link
Member

@mficzel mficzel left a comment

Choose a reason for hiding this comment

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

Not sure about that, fusion can technically return anything not only strings. I would rather remove the return type if there is no real reason for it.

Neos 5.1 & Flow 6.1 Release Board automation moved this from In progress to Review in progress Nov 9, 2019
@ComiR
Copy link
Contributor Author

ComiR commented Nov 11, 2019

@albe, @kitsunet, @kdambekalns, you were involved in the small discussion in Slack. What do you think?

@kdambekalns
Copy link
Member

It says string in the docblock, so it should be string. Or the docblock should be changed to say mixed, when the return tyepe is removed again.

@mficzel
Copy link
Member

mficzel commented Nov 14, 2019

I would prefer mixed as Fusion will often return DataStructures

@kitsunet
Copy link
Member

It really doesn't matter what fusion returns, if fluid says a viewhelper returns strings, then that's that. Although I currently think we have other examples that break that "rule".

@mficzel
Copy link
Member

mficzel commented Nov 14, 2019

If fluid expects viewHelpers to return strings i agree. This makes fluid pretty much unusable and inextensible to me but hey, i do use pure fusion anyways.

@mficzel
Copy link
Member

mficzel commented Nov 14, 2019

Looking at fluids code i just found that some helpers like integer (count), mixed (cycle,render,grouped) null(Variable) on renderStatic although the viewHelperInterface says renderStatic should return string.

In total i the fluid viewHelperInterface->renderStatic should be typed mixed and the result of the rendere viewHelper here aswell.

@ComiR
Copy link
Contributor Author

ComiR commented Nov 14, 2019

I now changed it to @return mixed. I followed the render method and changed the annotated return types there too.

@ComiR ComiR changed the title BUGFIX: Fusions RenderViewHelper must return string BUGFIX: Change return types of some Fusion methods to mixed Nov 14, 2019
Copy link
Member

@kitsunet kitsunet left a comment

Choose a reason for hiding this comment

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

I am fine with that for now! I guess we need to see what fluid does in the future :) THanks a lot

Neos 5.1 & Flow 6.1 Release Board automation moved this from Review in progress to Reviewer approved Nov 14, 2019
Copy link
Member

@mficzel mficzel left a comment

Choose a reason for hiding this comment

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

This totally makes sense and reflects how fusion is used. Thanks a lot

Copy link
Member

@albe albe left a comment

Choose a reason for hiding this comment

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

What was said above. Fluid ViewHelpers can indeed return something else than string, even though most docblocks suggest otherwise. See e.g. CountViewHelper which returns an int (yes, simple case).

Merging with previous approvals.

@albe albe merged commit ea545fa into neos:5.0 Nov 24, 2019
Neos 5.1 & Flow 6.1 Release Board automation moved this from Reviewer approved to Done Nov 24, 2019
@albe albe deleted the patch-11 branch November 24, 2019 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants