I haven't tested but I think the code is not correct. $category could not be an array as state in your code change (according to method document, it can only be CategoryNode|null|boolean CategoryNode object or null if $id is not valid)

I think you can change if (is_array($category)) to if ($category) and it should solve the issue

@joomdonation if we do that and Boolean is return the foreach will error.

We loop through an array and if it is not an array we cannot look so just get out if their.

Sorry, I don't get your last comment. I am talking about this command in the code you changed:

$category = \JCategories::getInstance($this->getName())->get($query['id']);

As I understand, it would never return an array, so your code if (is_array($category)) will always return false, the method getCategoryId will always return false and it is not correct (as I said, I haven't tested, just comment by reading code quickly)

Change it to if ($category) will make sure we only call the later code if a $category found and the fatal error should be sorted.

When I checked last night it was returning an array, I’ll check when I get home if your comment is valid.

You are right thank you, will send an update PR shortly. When checking last night I was looking at the getChildren return not the category ?

On Saturday, Dec 02, 2017 at 4:46 pm, Tuan Pham Ngoc <notifications@github.com (mailto:notifications@github.com)> wrote:

@joomdonation commented on this pull request.

In components/com_content/router.php (#18954 (comment)):

@@ -164,20 +164,23 @@ public function getCategoryId($segment, $query) { $category = JCategories::getInstance($this->getName())->get($query['id']); - foreach ($category->getChildren() as $child) + if (is_array($category))

I mean change this line to if ($category)

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub (#18954 (review)), or mute the thread (https://github.com/notifications/unsubscribe-auth/ABVglkP3HL7BV00if4CiMUGEo9i_UC_Wks5s8X8AgaJpZM4QzCin).

Hello all. Thanks for testing my initial Issue Report and then tackling this PR. I have found that your revised code works in some instances, but still fails to properly redirect non-authenticated users in some other instances.

Hopefully I can explain this one properly...

If I follow the steps above to reproduce and test the issue, the PR fixes things just fine ?

However, if a sub category is added to the mix, things don't work and this time it is a 404 - URL invalid error message seen. Here is what I did to test and get this new error...

Used the setup as described in the steps above. Then I created a new sub category (with Registered Access level) and put a new article in that new category. So my URL path that I am testing now looks like this...

domain.com/test-category-menu-item-alias/test-sub-category-alias/test-sub-category-article-alias

So in this new setup, the only thing with a menu item created is the original category. The sub category, and article within the sub category, do not have menu items.

New error message I see is 404 - URL invalid

Hmmm ok, we probably need to handle this higher up the chain? In the categories getInstance method.

I’ll have a deeper play with it

On Saturday, Dec 02, 2017 at 10:34 pm, Justin Herrin <notifications@github.com (mailto:notifications@github.com)> wrote:

Hello all. Thanks for testing my initial Issue Report and then tackling this PR. I have found that your revised code works in some instances, but still fails to properly redirect non-authenticated users in some other instances.

Hopefully I can explain this one properly...

If I follow the steps above to reproduce and test the issue, the PR fixes things just fine ?

However, if a sub category is added to the mix, things don't work and this time it is a 404 - URL invalid error message seen. Here is what I did to test and get this new error...

Used the setup as described in the steps above. Then I created a new sub category (with Registered Access level) and put a new article in that new category. So my URL path that I am testing now looks like this...

domain.com/test-category-menu-item-alias/test-sub-category-alias/test-sub-category-article-alias

So in this new setup, the only thing with a menu item created is the original category. The sub category, and article within the sub category, do not have menu items.

New error message I see is 404 - URL invalid

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub (#18954 (comment)), or mute the thread (https://github.com/notifications/unsubscribe-auth/ABVglgBGJXaLDo9UVHx_p1cy6NUA5wr1ks5s8dCAgaJpZM4QzCin).

Change the code from:

$category = JCategories::getInstance($this->getName())->get($query['id']);

To

$category = JCategories::getInstance($this->getName(), array('access' => false))->get($query['id']);

should help solving the issue. However, I am not sure what should be the right behavior (404 error or redirects to login page), so I will leave it to you to make decision

@Hackwar can we get your input on this please since you built the new router?

We can solve the first issue by redirecting to login as you would expected, but when handling more registered sub categories it gets more complex since the url is invalid for parsing.

The situation is valid, since you may provide clients/users with a hidden url which prompts for login to view it. It looks like an issue with the Parser.

URL routing should not care about access level. Access level will be checked later.

If so, I think the change I suggested before #18954 (comment) should solve the issue

They do not in my tests.

On 4 Dec 2017, 11:35 +0000, Tuan Pham Ngoc notifications@github.com, wrote:

If so, I think the change I suggested before #18954 (comment) should solve the issue
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

If so, I think the change I suggested before #18954 (comment) should solve the issue

Yes,

there is also another issue when a category is Archived.
What then? IMO the link also should work, but now the category is not found.

They do not in my tests.

=> Not sure if I miss any step but my test show that it works OK

there is also another issue when a category is Archived.
What then? IMO the link also should work, but now the category is not found.

We might want to pass published = 0 to $options array, so the code will be:

$category = JCategories::getInstance($this->getName(), array('access' => false, 'published' => 0))->get($query['id']);

However, it also return unpublished category which can cause potential issue.

@joomdonation can you do a PR please. I’ll close this.

OK, I created PR #18983 (which fixes the similar issues for other extensions, too)