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

Fixing missing alias in URLs #14322

Merged
merged 2 commits into from Mar 9, 2017

Conversation

Projects
None yet
7 participants
@Hackwar
Member

Hackwar commented Mar 4, 2017

Pull Request for Issue #9698.

Summary of Changes

The modern router currently has a slightly broken behavior regarding malformed router input. If a URL given to the router does not contain a complete slug, it will either create a URL without an alias (when it is supposed to create them with IDs), or it will create a URL with a proper alias, but also with an ID, even though removing IDs is enabled. This fixes that.

Testing Instructions

  1. Enable modern routing
  2. Create an article with links to another article, a contact and a newsfeed and, disabling the editor, edit all three URLs to only contain &id= instead of &id=:.
  3. Check in the frontend that the URL looks right except for the last segment, which is only the ID.
  4. Enable "Remove IDs from URL" for all three components and check again and see that now all three links have an alias, but also still contain the ID.
  5. Apply these changes
  6. Check again in the frontend and see that all three links are now correctly formed, containing just an alias and no ID.
  7. Disable the "Remove IDs from URL" for all three components and now see that all three URLs contain an ID and an alias.

Documentation Changes Required

No doc changes required.

@Hackwar

This comment has been minimized.

Member

Hackwar commented Mar 4, 2017

This should also fix #14308

@philip-sorokin

This comment has been minimized.

Contributor

philip-sorokin commented Mar 4, 2017

I have tested this item 🔴 unsuccessfully on a9781ac

JRoute::_() works fine, but the component router makes two database queries insted of single query. Please, pay attention that URLs are not going into cache in JRoute::_() after building.

components/com_content/router.php

if (!strpos($id, ':'))
{
    $db = JFactory::getDbo();
    $dbquery = $db->getQuery(true);
    $dbquery->select($dbquery->qn('alias'))
        ->from($dbquery->qn('#__content'))
        ->where('id = ' . $dbquery->q($id));
    $db->setQuery($dbquery);

    $id .= ':' . $db->loadResult();
    echo $id . '<br>';
}

components/com_contact/router.php

echo $id . '<br>';

Execute the code:

require_once JPATH_ROOT . '/components/com_content/helpers/route.php';
require_once JPATH_ROOT . '/components/com_contact/helpers/route.php';
require_once JPATH_ROOT . '/components/com_newsfeeds/helpers/route.php';
	
echo JRoute::_(ContentHelperRoute::getArticleRoute(8, 19)) . '<br>';
echo JRoute::_(ContactHelperRoute::getContactRoute(2, 34)) . '<br>';
echo JRoute::_(NewsfeedsHelperRoute::getNewsfeedRoute(2, 17)) . '<br>';

And you will see the result:

8:beginners
8:beginners
/using-joomla/extensions/components/content-component/article-category-list/beginners.html
2:webmaster
2:webmaster
/using-joomla/extensions/components/contact-component/contact-categories/park-site/webmaster.html
/using-joomla/extensions/components/news-feeds-component/news-feed-category/new-joomla-extensions.html
@Hackwar

This comment has been minimized.

Member

Hackwar commented Mar 4, 2017

@philip-sorokin I don't understand you. The URL is cached in JRouter. I don't know why the method apparently is called twice. But since this should be the exception and not the norm for a call to a component router, an additional, lightweight query doesn't seem like a problem to me.

@Hackwar

This comment has been minimized.

Member

Hackwar commented Mar 4, 2017

@infograf768 Please add that to the appropriate issue, for example #13978. It has nothing to do with this PR.

@philip-sorokin

This comment has been minimized.

Contributor

philip-sorokin commented Mar 4, 2017

@Hackwar

If I add a link in an article <a href="index.php?option=com_content&view=article&catid=19&id=8">Link</a>, it will pass through JRoute in the same way, so, it cannot be an exception. To build this link the component router will make 2 queries.

About caching. If I add a slightly modified link <a href="index.php?option=com_content&view=article&catid=19&id=8&start=3">Link</a>, the component router will make another unnecessary query (twice). What if I want to add 100 page links in an article?

And I am not talking about different links, but behaviour in this case the same.

@Hackwar

This comment has been minimized.

Member

Hackwar commented Mar 5, 2017

The URL normally contains the slug of the article. In that case no query is run. So when you deliberately leave that of, you have to run additional queries, since we otherwise don't know how to build the URL. Yes, slightly different URLs will not hit the cache. But that is because normally those point to somewhere else. Right now this is an acceptable trade-off for me.

@philip-sorokin

This comment has been minimized.

Contributor

philip-sorokin commented Mar 5, 2017

Normally, when you add an article link in your text editor, the link does not contain a slug, it contains only category and article IDs, and this is good, because you can change the article alias without editing this link placed in another article.

BTW, I noticed that some article links are built twice, while other only once, as expected. The link in my example is built twice.

Nothing criminal, but...

@Hackwar

This comment has been minimized.

Member

Hackwar commented Mar 7, 2017

I have extended the PR to fix another issue that came up with the list all categories view. See #13978 (comment) for the problem and testing instructions.

@AlexRed

This comment has been minimized.

Contributor

AlexRed commented Mar 7, 2017

I have tested this item successfully on 197e7d4


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

@infograf768

This comment has been minimized.

Member

infograf768 commented Mar 8, 2017

I have tested this item successfully on 197e7d4

Great! Works fine for me.


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

@infograf768

This comment has been minimized.

Member

infograf768 commented Mar 8, 2017

RTC.


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

@joomla-cms-bot joomla-cms-bot added the RTC label Mar 8, 2017

@jeckodevelopment jeckodevelopment added this to the Joomla 3.7.0 milestone Mar 8, 2017

@wilsonge wilsonge merged commit f94fb6c into joomla:staging Mar 9, 2017

4 checks passed

JTracker/HumanTestResults Human Test Results: 2 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

@joomla-cms-bot joomla-cms-bot removed the RTC label Mar 9, 2017

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