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

Changed the typehint of the EsiFragmentRenderer to the interface #10654

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
4 participants
@stof
Copy link
Member

commented Apr 8, 2014

Q A
Bug fix? maybe
New feature? maybe
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR n/a

While it is true that passing a different core renderer implementation as inline strategy does not make any sense. Typehint the inline strategy here makes it much more complex to decorate the inline strategy (your decorator has to extend the class and overwrite all public methods instead of implementing the interface directly just to pass the typehint). And I have a valid use case for it (which is the reason why I submitted it now), or rather @lolautruche has one in EZPublish: ezsystems/ezpublish-kernel#793

I don't know exactly whether this change should be considered as a bugfix (merged in the upcoming 2.3.13) or a new feature (merged in 2.5). I guess @lolautruche will like us if it goes its way to 2.3.x (EZPublish could bump its requirement to run on 2.3.13+ instead of 2.3.0+ and use a simpler code).

I remember suggesting the change in the initial PR btw, but it was rejecting sayign it does not make sense to use other strategies at that time.

@lolautruche

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2014

You guessed my thoughts! :-)
+1

@@ -32,10 +32,10 @@ class EsiFragmentRenderer extends RoutableFragmentRenderer
* The "fallback" strategy when ESI is not available should always be an
* instance of InlineFragmentRenderer.
*
* @param Esi $esi An Esi instance
* @param InlineFragmentRenderer $inlineStrategy The inline strategy to use when ESI is not supported
* @param Esi $esi An Esi instance

This comment has been minimized.

Copy link
@Tobion

Tobion Apr 8, 2014

Member

Esi|null

*/
public function __construct(Esi $esi = null, InlineFragmentRenderer $inlineStrategy)
public function __construct(Esi $esi = null, FragmentRendererInterface $inlineStrategy)

This comment has been minimized.

Copy link
@Tobion

Tobion Apr 8, 2014

Member

I'm ok with it but then the name should be more general like fallbackRenderer

This comment has been minimized.

Copy link
@stof

stof Apr 9, 2014

Author Member

well, the name is OK IMO, because the fact that the inlineStrategy is the one making sense here is still true, even if we allow decorating it

@stof stof added the HttpKernel label Apr 9, 2014

fabpot added a commit that referenced this pull request Apr 9, 2014

bug #10654 Changed the typehint of the EsiFragmentRenderer to the int…
…erface (stof)

This PR was submitted for the master branch but it was merged into the 2.3 branch instead (closes #10654).

Discussion
----------

Changed the typehint of the EsiFragmentRenderer to the interface

| Q             | A
| ------------- | ---
| Bug fix?      | maybe
| New feature?  | maybe
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

While it is true that passing a different core renderer implementation as inline strategy does not make any sense. Typehint the inline strategy here makes it much more complex to decorate the inline strategy (your decorator has to extend the class and overwrite all public methods instead of implementing the interface directly just to pass the typehint). And I have a valid use case for it (which is the reason why I submitted it now), or rather @lolautruche has one in EZPublish: ezsystems/ezpublish-kernel#793

I don't know exactly whether this change should be considered as a bugfix (merged in the upcoming 2.3.13) or a new feature (merged in 2.5). I guess @lolautruche will like us if it goes its way to 2.3.x (EZPublish could bump its requirement to run on 2.3.13+ instead of 2.3.0+ and use a simpler code).

I remember suggesting the change in the initial PR btw, but it was rejecting sayign it does not make sense to use other strategies at that time.

Commits
-------

d1fca90 Changed the typehint of the EsiFragmentRenderer to the interface

@fabpot fabpot closed this Apr 9, 2014

@lolautruche

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2014

Thanks ! 😃

@stof stof deleted the stof:fragment_renderer_typehint branch Apr 26, 2014

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.