Travis said No.

We have one Travis error

1) JComponentRouterRulesMenuTest::testPreprocessLanguage

Failed asserting that two arrays are equal.

--- Expected

+++ Actual

@@ @@

 Array (

-    'Itemid' => '47'

 )

/home/travis/build/joomla/joomla-cms/tests/unit/suites/libraries/cms/component/router/rules/JComponentRouterRulesMenuTest.php:218

@tonypartridge @wilsonge
Where do we go from here?

Someone needs to write the new tests, I think @mbabker has it on his list. But is crazy busy right now.

On 27 May 2017, 07:16 +0100, infograf768 notifications@github.com, wrote:

@tonypartridge @wilsonge
Where do we go from here?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

Someone needs to write the new tests

Not necessarily. We need to see if the test failures are a false positive (meaning the tests do need to be rewritten) or if the test failures indicate the code change is wrong and try a different one.

https://github.com/joomla/joomla-cms/blob/staging/tests/unit/suites/libraries/cms/component/router/rules/stubs/MockJComponentRouterRulesMenuMenuObject.php#L40

I mean the tests are never passing if none of the mock menu items have an access property which we're gonna be checking in this PR...

Oh, right, I vaguely remember that now.

So the issue is https://github.com/infograf768/joomla-cms/blob/cfc15ba104429b713719e1283f726faecff7925c/libraries/cms/component/router/rules/menu.php#L136 will never pass as true to assign the Itemid in that test case, the test is setting a null state for the active menu item (so there isn't one) and your added case requires that there be an active item.

So with this change as is, if you do not have an active menu item and you do not explicitly pass an Itemid as part of the query into JComponentRouterRulesMenu::preprocess(), you will not get an Itemid back at all. And it looks like the test behavior is intending to validate that if you reach the end of that preprocess function and you do not yet have an Itemid that you end up getting the default assigned as long as there is a default defined.

And FWIW, with SEF routes enabled, going through JRouterSite::parseSefRoute() it is indeed possible for the router to not call JMenu::setActive() (whereas JRouterSite::parseRawRoute() will always make that call). So it is a valid condition to test.

The problem was in multilingual you don't have an active menu item on the home page.

That case fails too (it'll show up better here in a moment since I turned that test case into using a data provider).

The preprocess method doesn't have a return value and it's argument is by
reference.

On Sun, May 28, 2017 at 8:49 AM Hannes Papenberg notifications@github.com
wrote:

@Hackwar commented on this pull request.

In
tests/unit/suites/libraries/cms/component/router/rules/JComponentRouterRulesMenuTest.php
#16233 (comment):

• // If we inject an item id and we have no active menu item we should get the injected item id
  

• $query = array('Itemid' => '50');
  

• $this->object->preprocess($query);
  

• $this->assertEquals(array('Itemid' => '50'), $query);
  

• // Test if the correct default item is used based on the language
  

• $query = array('lang' => 'en-GB');
  

• $this->object->preprocess($query);
  

• $this->assertEquals(array('lang' => 'en-GB', 'Itemid' => '51'), $query);
  

• // The argument here is by reference, so we'll validate the input matches expected
  

• $this->object->preprocess($input);
  

$input should not be handed over by reference and instead the return value
of preprocess should be used.

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#16233 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWfoSM8hKKVFamUXn8cAMpugbWSWrfsks5r-RkSgaJpZM4NkwPG
.

--

• Michael Please pardon any errors, this message was sent from my iPhone.

My bad, sorry, I mixed that up with JComponentRouterBase

All, I'm not sure if this is the right place to comment on the issue of the auto-redirect to the login page - my sincere apologies if not. I noticed this ticket, which I think is related to #16210, #16197, and #15730 is still open, so I thought it might be better than commenting on a closed ticket.

I upgraded one of my sites on a test environment to 3.7.3 this morning, but found that the auto-redirect to the login page (my Home and all other menus are set to "Registered") is still "broken." It's still appending the ID number of the Home menu to the end of the URL, like this:

mydomain.com/joomlasite/index.php/component/users/?view=login&Itemid=101

If I delete &Itemid=101 from the end of the URL, the login page appears. But, I can't log in.

Please note that I am not a developer, but I do administer 12 Joomla sites configured in this manner.

Hello,

Unfortunately my fix for that one was disregarded. @wilsonge was hoping to have fixed it. But it appears not

On 5 Jul 2017, 14:48 +0100, mattske notifications@github.com, wrote:

All, I'm not sure if this is the right place to comment on the issue of the auto-redirect to the login page - my sincere apologies if not. I noticed this ticket, which I think is related to #16210, #16197, and #15730 is still open, so I thought it might be better than commenting on a closed ticket.
I upgraded one of my sites on a test environment to 3.7.3 this morning, but found that the auto-redirect to the login page (my Home and all other menus are set to "Registered") is still "broken." It's still appending the ID number of the Home menu to the end of the URL, like this:
http://mydomain.com/joomlasite/index.php/component/users/?view=login&Itemid=101
If I delete &Itemid=101 from the end of the URL, the login page appears. But, I can't log in.
Please note that I am not a developer, but I do administer 12 Joomla sites configured in this manner.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@tonypartridge

shall I close this one?

Yeah please

On 5 Jul 2017, 15:35 +0100, infograf768 notifications@github.com, wrote:

@tonypartridge
shall I close this one?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

Done ?

@mattkse if you create a login menu item anywhere on your site then all will work again

Think a quick solution would be to add a check on Joomla! Admin menu to check if the menu item is not publically accessible and if not check if there is a login menu item if not put an announcement one is needed.

On 5 Jul 2017, 15:35 +0100, infograf768 notifications@github.com, wrote:

@tonypartridge
shall I close this one?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

If this is going to be closed please can we re-open an issue. Because this does need fixing. Unfortunately since JAB my time for Joomla has been limited because personal life, i've been slowly picking up on things the last week or so. This is a real issue though and it should be fixed for 3.8

@wilsonge fully understand :) same here too. What about my suggest for a check similar to the php check and then if it is a problem offer a one click button to create a login menu item with display in menu set to no?

On 5 Jul 2017, 15:54 +0100, George Wilson notifications@github.com, wrote:

If this is going to be closed please can we re-open an issue. Because this does need fixing. Unfortunately since JAB my time for Joomla has been limited because personal life, i've been slowly picking up on things the last week or so. This is a real issue though and it should be fixed for 3.8
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

Thanks @tonypartridge, I added a login menu with Display In Menu set to No. Works fine... is an easy solution to implement for my sites.

@mbabker @wilsonge Thoughts on my proposed solution?

Add a new home page checker, operates similar to our php check? but then provide a button to auto-create the hidden log-in menu item on the main menu?

I'd much rather fix it properly. Whilst we need to encourage people to create quality menu structures it shouldn't be completely compulsory!