Skip to content

[Modern Routing] make a simpler loop in StandardRules::build#19012

Closed
csthomas wants to merge 3 commits intojoomla:stagingfrom
csthomas:make_simpler_standard_rule_build
Closed

[Modern Routing] make a simpler loop in StandardRules::build#19012
csthomas wants to merge 3 commits intojoomla:stagingfrom
csthomas:make_simpler_standard_rule_build

Conversation

@csthomas
Copy link
Copy Markdown
Contributor

@csthomas csthomas commented Dec 7, 2017 •
edited
Loading

Summary of Changes

Replace the old complex loop with a simpler one.
Remove PHP notice.
Everything should work as before.

This PR adds a way to build correct URL for below structure:

$featured = new JComponentRouterViewconfiguration('featured');
$this->registerView($featured);
//$categories = new JComponentRouterViewconfiguration('categories');
//$categories->setKey('id')->setParent($featured);
//$this->registerView($categories);
$category = new JComponentRouterViewconfiguration('category');
$category->setKey('id')->setParent($featured)->setNestable()->addLayout('blog');
$this->registerView($category);
$article = new JComponentRouterViewconfiguration('article');
$article->setKey('id')->setParent($category, 'catid');
$this->registerView($article);
$this->registerView(new JComponentRouterViewconfiguration('archive'));
//$this->registerView(new JComponentRouterViewconfiguration('featured'));
$form = new JComponentRouterViewconfiguration('form');
$form->setKey('a_id');
$this->registerView($form);

Testing Instructions

Install Joomla with sample data testing.
After you replace php code in com_content/router.php as describer above:

  • disable all menu items for categories and category view.
  • add a featured view to menu item if missing
  • Go to index.php/component/content/categories/ and compare link to category view before and after patch.

Expected result

No PHP notices.
Link to the top category view should be like
/featured-articles/14-sample-data-articles

Actual result

There is a PHP Notice: Undefined index id on line 265.

Documentation Changes Required

No

@csthomas csthomas changed the title [Modern Routing] make loop simpler in StandardRules::build [Modern Routing] make a simpler loop in StandardRules::build Dec 7, 2017
@ghost
Copy link
Copy Markdown

ghost commented Dec 9, 2017

After you replace php code in com_content/router.php as describer above:

this is be done by applying PR or manually?

@csthomas
Copy link
Copy Markdown
Contributor Author

csthomas commented Dec 9, 2017

Manually,
to test this PR you need a custom routing configuration as above.
StandardRules from modern routing should support a various configurations, above too.

To create a test example (in order to not install a separate component) I created a custom configuration for com_content in order to show a bug in StandardRules build method.

For such custom configuration, the link to the top category should look like /featured-articles/14-sample-data-articles.

You do not need to click on that link because it has not work yet (parse method has not support such custom configuration yet) and returns error 404. I would like to leave it for another PR.

For this PR I would like to change the loop to simpler and remove PHP notice.

@joomdonation As you are familiar with StandardRules class, would you like to to add your opinion?

@ghost
Copy link
Copy Markdown

ghost commented Dec 9, 2017

@csthomas this is above my skills.

@csthomas
Copy link
Copy Markdown
Contributor Author

csthomas commented Dec 9, 2017

But thanks for interest:)

@fancyFranci
Copy link
Copy Markdown
Contributor

fancyFranci commented Dec 11, 2017

I have tested this item ✅ successfully on bf28ce6

After following the instructions the top link has the url "/featured-articles/sample-data-articles" instead of the php notice about an undefined id index.
Like expected the link leads to error 404. With IDs in url its workting too.

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

joomdonation
joomdonation reviewed Dec 18, 2017
View reviewed changes
Comment thread libraries/src/Component/Router/Rules/StandardRules.php
$view = $views[$element];

if ($found)
if ($found === false && $item && $item->query['view'] === $element)
Copy link
Copy Markdown
Contributor

@joomdonation joomdonation Dec 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think $item check is not needed here as we check and make sure it is not null in previous code already

Copy link
Copy Markdown
Contributor Author

@csthomas csthomas Dec 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

@joomdonation
Copy link
Copy Markdown
Contributor

joomdonation commented Dec 18, 2017

@csthomas I think for this PR you should focus in make a simpler loop in StandardRules::build task only. That part is good and easier to understand compare to the original code. It will help testers easier to review the change and also easier for maintainer to review and merge.

For adding support for new router configuration, I would leave it for the next PR as I am unsure if it needed for now (and as you mentioned, the generated link causes 404 error anyway). The routing code is quite complex, so I prefer small step at a time.

@csthomas
Copy link
Copy Markdown
Contributor Author

csthomas commented Dec 29, 2017

Ok. When I have more time to think about it I will fix it.

@csthomas csthomas mentioned this pull request Jan 3, 2018
Merged
@csthomas
Copy link
Copy Markdown
Contributor Author

csthomas commented Jan 3, 2018

I close it in favour of #19271

@csthomas csthomas closed this Jan 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@joomdonation joomdonation joomdonation left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@csthomas @fancyFranci @joomdonation @joomla-cms-bot