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

Generalized Route::getTargetPresenter() #40

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

JanTvrdik
Copy link
Contributor

Allow using this optimization for other IRouter implementations.

Ideas for better names of anything are welcomed.

@hrach
Copy link
Contributor

hrach commented Nov 2, 2014

static?

@JanTvrdik
Copy link
Contributor Author

We may implement the getTargetPresenters as well it for RouteList as well.

    function getTargetPresenters()
    {
        if ($this->cachedRoutes === NULL) $this->cachedRoutes = $this->buildCache();
        return empty($this->cachedRoutes['*']) ? array_keys($this->cachedRoutes) : NULL;
    }

@mishak87
Copy link

mishak87 commented Nov 2, 2014

Sorry about previous comments. Without further change in RouteList it is major BC break for multiple presenters in single route?

@JanTvrdik
Copy link
Contributor Author

@mishak87 Sorry, I have no idea what you mean. Please rephrase. This feature should be entirely without BC break.

@mishak87
Copy link

mishak87 commented Nov 2, 2014

Original: FALSE skips adding anything, NULL adds default values and string one presenter if the item is Route instance.

Change: Adds interface returning multiple routes and removes FALSE branch while also renaming method to plural.

@JanTvrdik
Copy link
Contributor Author

I still don't know what you are talking about. Are you suggesting something? Informing me of something? Are you stating facts?

@dg
Copy link
Member

dg commented Nov 4, 2014

He means that you changed getTargetPresenter -> getTargetPresenters and it's return value. But getTargetPresenter is internal method, so it is not BC break.

@JanTvrdik
Copy link
Contributor Author

@dg Does this makes sense to you? Should I finish the implementation (e.g. write tests)?

@JanTvrdik
Copy link
Contributor Author

ping

@JanTvrdik
Copy link
Contributor Author

Even after more than one month I still think this is a great idea. Except for the interface name, that is terrible. cc @dg

@fprochazka
Copy link
Contributor

I'm not sure about the interface name, but I like the idea. 👍

@dg
Copy link
Member

dg commented Jan 18, 2015

I think that getTargetPresenters can be part of IRouter.

@JanTvrdik
Copy link
Contributor Author

That would solve the ugly IFixedTargetPresenters name. But it would be a nasty BC break. It would actually be so large BC break that if we choose to do it, we may as well break other things (I have few ideas).

@dg
Copy link
Member

dg commented Jan 18, 2015

I would prefer to avoid BC break. This method can be in interface in v2.3 commented out and checked via method_exists.

@JanTvrdik
Copy link
Contributor Author

That seems reasonable.

@dg
Copy link
Member

dg commented Jan 18, 2015

Typo in commit message: IRouter

@JanTvrdik
Copy link
Contributor Author

Yeah and it also does not have tests so we don't know whether it actually work. And I don't feel like writing them right now.

@JanTvrdik JanTvrdik changed the title Route: getTargetPresenter() generalized to interface IFixedTargetPresenter Generalized Route::getTargetPresenter() Jan 18, 2015
dg added a commit that referenced this pull request Jan 26, 2015
dg added a commit that referenced this pull request Jan 26, 2015
@JanTvrdik
Copy link
Contributor Author

Do you plan to merge this to 2.3?

@dg
Copy link
Member

dg commented Feb 1, 2015

Yes, will you finish it?

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.

6 participants