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

Routing: Legacy Router Rule for com_newsfeeds #5503

Closed
wants to merge 60 commits into from

Conversation

Hackwar
Copy link
Member

@Hackwar Hackwar commented Dec 23, 2014

This implements the current routers behavior of com_newsfeeds as a legacyrouter. This is meant for those that want to keep the old behavior at all costs. Later, parameters will be introduced to switch between the legacy behavior and new features.

This router is copied verbatim from the current router to make sure that the same behavior is kept. No changes should be done to it until Joomla 4.0, when this is supposed to be deleted. The component routers right now are so fragile, that a slow transition will be next to impossible. To prevent any breaks in backwards compatibility, this legacy router is introduced, so that people having problems with the new router can switch back.

How to test

  1. Check your site before applying the patch and see the URLs of com_newsfeeds.
  2. Apply the patch.
  3. See the URLs of com_newsfeeds to stay the same.

Dependencies

This PR requires #5446 to be accepted by the project before it can be applied.

This was made possible through the generous donation of the people mentioned in the following link via an Indiegogo campaign: http://joomlager.de/crowdfunding/5-contributors

Hackwar and others added 17 commits March 26, 2015 01:02
Call-time pass-by-reference has been removed

Removing JError, using Exception instead

protecting $name and renaming register() to registerView()

Adding removeRule, getRules and renamed $id to $key in register method

Making method names consistent

Implementing JComponentRouterViewconfiguration for configuration of views in JComponentRouterAdvanced

Codestyle, smaller improvements, unittests for all component router classes except for JComponentRouterAdvanced

Removing ability to have one view with different names and implementing unittests for JComponentRouterAdvanced

Adding get<View>Slug() and get<View>Key() methods to JComponentRouterAdvanced

Updating unittest

Small fixes

Adding back in platform check

Adding back in platform check

Adding back in platform check

Adding back in platform check

Implementing feedback so far

Adding "covers" notation for unittests
…omponentrulesitemid

Conflicts:
	tests/unit/suites/libraries/cms/component/router/JComponentRouterViewTest.php
Fix unit test failure in JComponentRouterViewTest
…into com_newsfeed_router_legacy

Conflicts:
	components/com_newsfeeds/helpers/route.php
@joomla-cms-bot joomla-cms-bot added the Language Change This is for Translators label May 27, 2015
@Hackwar
Copy link
Member Author

Hackwar commented May 27, 2015

This has been updated to work with the latest implementation of JComponentRouterView. Please test.

@Bakual
Copy link
Contributor

Bakual commented May 27, 2015

Same here, looks like you rebased/merged using the 3.5 branch instead of the staging branch?

@Hackwar
Copy link
Member Author

Hackwar commented May 27, 2015

No, this is expected. The original PR was from a time where no 3.5-dev branch existed. Since the basic classes were merged into the 3.5-dev branch, I had to pull in the 3.5-dev branch into this one to make everything work. Since we can't switch branches for PRs after they have been created, I can't correct that. But overall, this is perfectly fine this way.

@Bakual
Copy link
Contributor

Bakual commented May 27, 2015

If you want to do the PR against the 3.5 branch, you can close this PR and create a new one. You can do that from the existing branch. Doing it like you did doesn't work.

@Hackwar
Copy link
Member Author

Hackwar commented May 27, 2015

#5444 was against staging and was merged by you guys into 3.5-dev. The other PRs are depending on that code and thus I need to merge that code into this PR. I did discuss this with George. I know how to use git and this here was the reason why I proposed to use a different branching scheme than the current one.

If we were to create a branch for the current minor release and let people create bugfixes against that branch instead of creating a branch for the NEXT minor release and thus force ALL PRs that are meant to go into a minor release to be closed and re-opened or manually merged, we could develope the future version of Joomla in staging and bug-releases would really just be bug-releases, because only serious issues/regressions would be solved by PRs that are targeted at the current minor release branch. But the way it is now, a feature that I'm working on (like for example the routing) would either have to be aimed at staging and then create this mess that we have right now, or you would have to open it against the next minor release branch, then see it not getting merged, close it, open it against the then next minor release branch, again not seeing it being merged, closing, re-opening and then hoping that people will finally get this.

@Bakual
Copy link
Contributor

Bakual commented May 27, 2015

Your PRs are quite a special case.as they depend on eachother. We really have those seldom.

@wilsonge I leave that and #5502 for you to deal with.

As it is now, it's impossible to test and/or review properly as it's targeting another branch than it's based on. Esentially when merging this we would merge 3.5-dev into staging.

@Hackwar
Copy link
Member Author

Hackwar commented Aug 2, 2015

I've combined the changes from this and all other routing related PRs into a new PR: #7615 Please review and comment in the new PR. I'm closing this one, so that we can focuse on the new PR.

@Hackwar Hackwar closed this Aug 2, 2015
@Hackwar Hackwar deleted the com_newsfeed_router_legacy branch January 6, 2016 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Language Change This is for Translators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants