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
[com_content] Fix layout in category link #20200
Conversation
|
||
if ($layout !== '') | ||
$layout = JFactory::getApplication()->input->get('layout', '', 'string'); | ||
if (!empty($layout)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add blank line above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -79,10 +79,8 @@ public static function getCategoryRoute($catid, $language = 0) | |||
$link .= '&lang=' . $language; | |||
} | |||
|
|||
$jinput = JFactory::getApplication()->input; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO 'cmd' should be better than 'string'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if CMD layout was templatelayout
and bug still alive if string template:layout
this bug was on my site =)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the layout can contain colon, then yes, I'm wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@csthomas You were not wrong, just a filter designed for parameters why ignores the colon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pattern for CMD is $pattern = '/[^A-Z0-9_\.-]/i';
.
There could be a two options: add colon to filter code or replace colon to dot in layout.
The same as work for task=controller.method
.
At now you do not have an option, stay with string.
$jinput = JFactory::getApplication()->input; | ||
$layout = $jinput->get('layout'); | ||
|
||
if ($layout !== '') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just a cosmetic suggestion. Instead of if (!empty($layout))
would be better to use if ($layout)
. We do not need to check if $layout
is set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, I somehow did not even think. Done
Never mind the code style has anyone replicated the bug as I could not
…On 18 Apr 2018 10:14 pm, "Tomasz Narloch" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In components/com_content/helpers/route.php
<#20200 (comment)>:
> @@ -79,10 +79,8 @@ public static function getCategoryRoute($catid, $language = 0)
$link .= '&lang=' . $language;
}
- $jinput = JFactory::getApplication()->input;
The pattern for CMD is $pattern = '/[^A-Z0-9_\.-]/i';.
There could be a two options: add colon to filter code or replace colon to
dot in layout.
The same as work for task=controller.method.
At now you do not have an option, stay with string.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#20200 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABPH8fHbx8LQi1JPKgMxTgtrnrtXDEYmks5tp6yggaJpZM4TavDK>
.
|
@brianteeman I, too, failed to reproduce the bug on the newly installed joomla, can only suggest creating an alternative / override layout for the test But I can provide details of why #19681 caused a bug In the past, PR layout turned out so
What led to the following results
This code will correctly add layout to the link
In fact in this PR, I just corrected what was added by the filter 'string' and changing the check |
|
I have tested this item ✅ successfully on f8ad03c This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20200. |
@Quy I tried to reproduce this bug with a clean installation of joomla on one of my subdomains, But I could not reproduce the bug. Thank you. |
I have tested this item ✅ successfully on f8ad03c This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20200. |
RTC This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20200. |
In fact, I would delete the addition of layout to the link, because personally I think it is not correct #19681 PR. Because if I choose another for another language I choose another layout. Maybe I need to have another layout. If there are those who think the same way, Say and I just delete the layout from the formation of the link in this PR |
Pull Request for Issue #20199.
Summary of Changes
Fix bag with
?layout=
in com_content category link after #19681Testing Instructions
If you have this bug, apply the patch
How reproduce bug by @Quy
Modern
Park Blog
http://localhost/joomla387/index.php/using-joomla/extensions/components/content-component/article-category-blog?layout=
Expected result
Link was
/category
Actual result
Now link
/category?layout=
or/category?layout=templatelayout
if override com_content category view