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

Jroute between sites #16879

Merged
merged 31 commits into from Mar 9, 2018

Conversation

@izharaazmi
Contributor

izharaazmi commented Jun 27, 2017

Redo of #11060

Most of the work was originally done by @andrepereiradasilva. This is a redo of his work and more additions from me.

Summary of Changes

This PR is for allowing JRoute to generate URL's from admin to site and vice-versa.

This is a current problem especially if you are trying to make links from the backend to the frontend.

So, with this PR, there is no need anymore to make special code just to get the frontend link from the backend. With this PR we just need to use JRoute, like all other links. The same will work to links from frontend to backend. After this PR, no need to hardcode frontend to admin links.

We will be able to use following syntax:

// Build a frontend URL irrespective of in which context (site/admin) the code is running
JRoute::link('site', $url, $xthml = true, $ssl = null);

// Build a backend URL irrespective of in which context (site/admin) the code is running
JRoute::link('administrator', $url, $xthml = true, $ssl = null);

This should not affect the existing JRoute::_() method.

Testing Instructions

  • Test various URLs for different extensions (articles, menu items, etc).
  • You should try with the several combinations of site settings like:
    • SEF/non-SEF
    • With or without URL rewriting
    • With or without multi-language (i.e. with or without language filter plugin enabled).

Test 1: Links from admin to site

  • Use latest staging with sample data from at least two languages (en-GB, fr-FR)
  • Add to isis template index.php file...
$app = JFactory::getApplication();
$app->enqueueMessage('1. Admin to Admin: ' . JRoute::_('index.php'));
$app->enqueueMessage('2. Admin to Admin: ' . JRoute::_('index.php?option=com_content'));
$app->enqueueMessage('3. Admin to Site: ' . JRoute::link('site', 'index.php?option=com_content'));
$app->enqueueMessage('4. Admin to Site (MutilLanguage 1): ' . JRoute::link('site', 'index.php?option=com_content&lang=fr'));
$app->enqueueMessage('5. Admin to Site (MutilLanguage 2): ' . JRoute::link('site', 'index.php?option=com_users&lang=en'));
  • Open any admin page. Check the URL in resulting message are correct.

Test 2: Links from site to admin

  • Add to protostar template index.php file...
$app = JFactory::getApplication();
$app->enqueueMessage('1. Site to Site: ' . JRoute::_('index.php'));
$app->enqueueMessage('2. Site to Site: ' . JRoute::_('index.php?option=com_content'));
$app->enqueueMessage('3. Site to Admin: ' . JRoute::link('administrator', 'index.php?option=com_content'));
  • Open any frontend page. Check the URL in resulting message are correct.

Test 3: Code Review

Do a code review.

Test 4: Current URLs

Check if current URL generated by JRouter are exactly the same, i.e. no change in current URL.
Probably the best is to leave the patch applied for some time, while you do other things and check there are no problems with the current URLs.


Thanks to @mbabker, @ggppdk and @infograf768 for helping out with this one.

Maintainers, to make sure that are no unintended consequences it would be good if this can be also reviewed from someone that understand the Router better than I do.

return static::_($arguments[0], $xhtml, $ssl, $name);
}
return null;

This comment has been minimized.

@mbabker

mbabker Jun 27, 2017

Member

This should be a trigger_error() call because you in essence have a "call to undefined function" error here.

This comment has been minimized.

@izharaazmi

izharaazmi Jun 27, 2017

Contributor

If we check for a valid client name here, the behavior would be different when called using undescore function. That function fails silently on invalid router and we need to keep it like that for B/C, imho.

And here we are only checking for missing first parameter coz we need to populate 4th parameter.

Another alternate way that comes to my mind is to not modify the undescore function signature and add another common function that accepts client name as first parameter.

We might then not need the magic function too. Your opinion please.

This comment has been minimized.

@mbabker

mbabker Jun 27, 2017

Member

Personally I hate the use of magic functions. They usually expose unwanted behavior and do not handle error conditions appropriately (in an error state they should do the same thing as the native PHP engine does for whatever that magic function is overloading). Even in the Framework's database repo, I removed __call() and just made "real" stub methods for the shorthand q(), qn(), and e() methods.

So personally I'd just add stub real methods and avoid the magic as a first option. If the magic method is really needed, then it needs to error trap/handle properly.

That function fails silently on invalid router and we need to keep it like that for B/C, imho.

If the system can't generate a route, it shouldn't silently fail over. There needs to be an error raised.

$router->attachBuildRule(array($this, 'preprocessBuildRule'), JRouter::PROCESS_BEFORE);
$router->attachBuildRule(array($this, 'buildRule'), JRouter::PROCESS_DURING);
// We need to make sure we are always using the site router, even if the language plugin is executed in admin app.
$router = $this->app->isClient('site') ? $this->app->getRouter() : JApplicationCms::getInstance('site')->getRouter('site');

This comment has been minimized.

@mbabker

mbabker Jun 27, 2017

Member

Just have this always call JApplicationCms::getInstance('site')->getRouter('site'), the singleton instance in JApplicationCms will be the same as the one in JFactory::$application if you're on the frontend anyway.

This comment has been minimized.

@izharaazmi

izharaazmi Jun 27, 2017

Contributor

Sure.

@mbabker mbabker added this to Testing/Review in [3.8] Routing Jun 27, 2017

@izharaazmi

This comment has been minimized.

Contributor

izharaazmi commented Jun 27, 2017

@mbabker Please review the changes now. I have kept the magic method and it does not ignore any errors. If you suggest I can also remove that altogether.

*
* @since __DEPLOY_VERSION__
*/
public static function link($client, $url, $xhtml = true, $ssl = null)

This comment has been minimized.

@wilsonge

wilsonge Jun 27, 2017

Contributor

This signature should be public static function link($url, $client=null, $xhtml = true, $ssl = null) (we should be explicit the default client variable is null rather than making it compulsory and then passing in null in JRoute::_)

This comment has been minimized.

@izharaazmi

izharaazmi Jun 27, 2017

Contributor

From where I see this, the undescore function is the dedicated interface for the cases where the programmer wants to use active client.

This method link() should ideally be used in cases where the specific $client value has the main significance. It will also improve the readability of the code as a bonus.

Therefore, IMO, this signature looks fit for the purpose. If someone still insist on using link() for what _ is meant for, then we've no reason to stop them.

This comment has been minimized.

@mbabker

mbabker Jun 27, 2017

Member

Then the link method shouldn't have support for injecting $client = null. That logic should move into _.

This comment has been minimized.

@izharaazmi

izharaazmi Jun 27, 2017

Contributor

Yeah that makes sense. I'll update it this afternoon.

This comment has been minimized.

@izharaazmi

izharaazmi Jun 28, 2017

Contributor

Done.
The Travis failure is probably something unrelated.

This comment has been minimized.

@mbabker

mbabker Jun 30, 2017

Member

Can you check just to make sure? The builds for this pull request didn't start failing until this last change.

This comment has been minimized.

@wilsonge

wilsonge Jul 2, 2017

Contributor

These tests are failing with Routing related issues. I think they are as a result of this PR

@mbabker mbabker changed the base branch from staging to 3.8-dev Jul 2, 2017

@mbabker

This comment has been minimized.

Member

mbabker commented Jul 2, 2017

I started digging into the test problems. We're in dependency hell.

Problem 1 - TestMockApplicationCms::getMethods() needs to include the getName method
Problem 2 - For JPaginationTest, need to add this line to configure the application mock in the setUp method (note that there will be some places that need to change the mock return to administrator in that test class)

$app->expects($this->any())->method('getName')->willReturn('site');

Problem 3 - JApplicationCms::getRouter() is static, it cannot be mocked, so we have to work with a real router (or use Reflection and push a mock into the JRoute::$_router property)
Problem 4 - JRouter::getInstance() does not support constructor injection of the application and menu objects for the JRouterSite subclass, so there is no way to inject stuff, meaning that code is falling onto the defaults
Problem 5 - The JRouterSite constructor calls JApplicationCms::getInstance('site'), this tries to create a JApplicationSite object with default behaviors since there isn't a mock application set up in JApplicationCms::$instances, and this ultimately leads up to a session start failure
Problem 6 - I got too deep into this and said "no more" 😆

@wilsonge

This comment has been minimized.

Contributor

wilsonge commented Jul 2, 2017

So if you change https://github.com/izharaazmi/joomla-cms/blob/c7dd17d70bebe41230c28140612eccce5d727506/libraries/joomla/application/route.php#L87 to be if (!isset(self::$_router[$client])) (i.e. allow $client to be null), then unit tests pass and effectively replicate the current behaviour (admittedly the incorrect behaviour).

@mbabker

This comment has been minimized.

Member

mbabker commented Jul 2, 2017

Well, good thing JRouter isn't abstract then.

@mbabker

This comment has been minimized.

Member

mbabker commented Aug 15, 2017

In theory that article ID 17 should still get a SEF URL unless all of its parent categories too have no menu item (or its only parent is a featured or archived view).

@izharaazmi

This comment has been minimized.

Contributor

izharaazmi commented Aug 15, 2017

@Fedik

This comment has been minimized.

Contributor

Fedik commented Aug 15, 2017

It is I think.

Yeap, I just tried default method JRoute::_( 'index.php?option=com_content&view=article&id=17'); at the site, and got same result.

As it do not related to the current patch, I am going to mark it as success.

@Fedik

This comment has been minimized.

Contributor

Fedik commented Aug 15, 2017

I have tested this item successfully on b3136c4


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16879.

@sanderpotjer

This comment has been minimized.

Member

sanderpotjer commented Aug 30, 2017

I have tested this item successfully on b3136c4

@izharaazmi & @andrepereiradasilva great PR, have been using a hacky way so far to generate frontend SEF links from the backend, so much welcome :-)

I have been playing around with the patch, and for all links I tried the output was similar in the frontend as in the backend. Tried varies combinations, SEF on/off, htaccess on/off, stable/experimental router. So seems to work correctly!


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16879.

@izharaazmi

This comment has been minimized.

Contributor

izharaazmi commented Aug 31, 2017

@mbabker Its post beta4 now. Is there still any change getting this in?

@izharaazmi

This comment has been minimized.

Contributor

izharaazmi commented Sep 15, 2017

Conflicts resolved.

@euismod2336

This comment has been minimized.

Contributor

euismod2336 commented Sep 29, 2017

I have tested this item successfully on b3136c4

Love this PR.

Both in frontend as backend the expected url's show up, both with SEF enabled and disabled.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16879.

@franz-wohlkoenig

This comment has been minimized.

franz-wohlkoenig commented Sep 29, 2017

RTC after two successful tests.

@joomla-cms-bot joomla-cms-bot added the RTC label Sep 29, 2017

@mbabker mbabker changed the base branch from staging to 3.9-dev Oct 15, 2017

@mbabker mbabker added this to the Joomla 3.9.0 milestone Oct 15, 2017

@ctech-design

This comment has been minimized.

ctech-design commented Feb 9, 2018

I have tested this and really need this PR to be added in Joomla soon, It's been month how long do we have to wait. Please consider releasing this soon

@tonypartridge

This comment has been minimized.

Contributor

tonypartridge commented Feb 9, 2018

@asfaqueali

Unfortunately this cannot be added into joomla! Until 3.9 since new features cannot go into 3.8 as that would break versioning.

@ctech-design

This comment has been minimized.

ctech-design commented Feb 9, 2018

@tonypartridge yeah I understand, but its been almost 4 months and release date of 3.9 is not defined so far.

@tonypartridge

This comment has been minimized.

Contributor

tonypartridge commented Feb 9, 2018

@wilsonge wilsonge merged commit 8a51f23 into joomla:3.9-dev Mar 9, 2018

5 checks passed

JTracker/HumanTestResults Human Test Results: 3 Successful 0 Failed.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
hound No violations found. Woof!

@joomla-cms-bot joomla-cms-bot added PR-3.9-dev and removed RTC labels Mar 9, 2018

@wilsonge

This comment has been minimized.

Contributor

wilsonge commented Mar 9, 2018

Merged - awesome work :)

@ctech-design

This comment has been minimized.

ctech-design commented Mar 9, 2018

@tonypartridge @wilsonge Thanks for merging it. @izharaazmi thanks for this commit.

@tonypartridge

This comment has been minimized.

Contributor

tonypartridge commented Mar 9, 2018

@izharaazmi izharaazmi deleted the izharaazmi:jroute-between-sites branch Mar 12, 2018

@brianteeman brianteeman removed the PR-staging label Mar 17, 2018

@mbabker mbabker referenced this pull request May 9, 2018

Merged

[Feature] Tracking Information Requests #15

4 of 6 tasks complete

mbabker added a commit to mbabker/joomla-cms that referenced this pull request May 12, 2018

Jroute between sites (joomla#16879)
* allow to force site client when we are in admin

* allow from site to admin too

* first try to make language filter router work on admin too when called via JRoute

* revert

* revert 2

* make language filter pluign work with JRoute from admin

* cs

* cs 2

* use app

* solve php notices

* Few tweaks for the PR

* Suggestions from @mbabker, and few other improvements.

* Moved active client detection logic to JRoute::_() and disallow null client value in JRoute::link()

* Expand mocking in JHtmlBehaviorTest

* Mocking in JHtmlIconsTest

* Fix reset of router property

* Mocking in JPaginationTest

* Mocking in JDocumentOpensearchTest

* Mocking in JDocumentRendererHtmlModulesTest

* Mocking in JFormFieldRulesTest

* Test fixes

* Moved SEF mode detection to CMSApplication for it being called from all application instances.

* Remove magic methods to force using of JRoute::link() directly

@wilsonge wilsonge moved this from Testing/Review to Completed in [3.9] General Aug 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment