User tests: Successful: Unsuccessful:
You can use the same steps to reproduce the issue #5215.
After applying the patch you see an error message when you want to save the category with the name 1.2.3 (or if you want to save another alias that starts with a number and contains no letter).
Please test also what happens if you want to save another item with an alias starting with a number and containing no letter. I applied this error message for menus and content items to.
Make sure that your Article Manager | Options | Category | Empty Categories, is set to Show.
Should display an empty category list.
You get a 404 error.
LAMP stack.
Joomla 3.3.6
If you put any text character at the start of the alias string (eg. v1-2-3 instead of just 1-2-3) the problem goes away.
Summary
Alias should not allow only numbers.
Status | New | ⇒ | Pending |
Labels |
Added:
?
?
|
I have tested this item successfully on 2529697
Patch works like expected but i think numbers only parts in url should be allowed, no?
This PR has received new commits.
CC: @designbengel, @matrikular
While I did this I was not sure if it would be better to change the test in the if clause in: Test if there is one letter in the alias (if (preg_match('/[a-zA-Z]/', $this->alias) === 0)).
What do you think about this.
question is, why it´s not working with numbers?
This PR has received new commits.
CC: @designbengel, @matrikular
@matrikular
I do not create the error message in this patch. I only set the error text. The creation of the error is done by the same task that created the “Save failed with the following error: Another article from this category has the same alias (remember it may be a trashed item).”Message and this message has the same colors.
So, a category alias starting with a number has a red error type message and in com_menu, it is a warning in yellow in the case the alias already exists.
@designbengel
Yes, you are right. The question is, why it´s not working with numbers? It depends on the router and the router. But I am not deep enough in Joomla to understand the router.
My solution is not the best way to fix this problem. You need to fix the router. But it is a pragmatic way to solve this issue.
Milestone |
Added: |
Milestone |
Added: |
||
Status | Pending | ⇒ | Ready to Commit |
Labels |
Category | ⇒ | Administration |
RTC. I have alterd the tests as we just got some CS fixes since the tests Thanks.
Labels |
Added:
?
|
@zero-24
Thank you for helping :-)
@designbengel @matrikular
Thank you for testing :-)
This is backwards incompatible and will break thousands of sites. Yes, maybe your specific case has an issue (1.2.3) and there is an error there, but forcing the alias to start with a letter and not with a number is a BIG mistake. I have dozens of sites that work with menu items that are just the year and they work very fine. With your patch all those menu items would be impossible to edit anymore.
Please investigate why you get an alias of 1-2.3 and not 1.2.3 when creating your items. That looks more like the issue. However this patch will create BIG problems.
@Hackwar
Thank you for having a look over this issue. I know you have the knowledge of the router :)
But users can edit sites with an alias starting with a number any more. They only have to edit this alias … and then they have maybe to change some more
But I think the bug is also not good.!
What do you success?
Alter I made this PR, I had another idea. I want to check if there is a letter in the alias. Would this solution be better?
Milestone |
Removed: |
||
Status | Ready to Commit | ⇒ | Pending |
Labels |
Milestone |
Removed: |
Thanks hannes just moved back to pending.
Labels |
Removed:
?
|
I disagree that it would break thousands of sites. It actually wouldn't break a single site right away.
However it's correct that when editing an item (mainly content, category and menuitem), you would have to change the alias (and thus the URL).
Especiall for the menu items, we have cases where we currently store the datetime. This is mainly for the menutype "Menu Item Alias" and "External URL" where the alias actually is never used. So it has to work with those or we have to change alias creation there.
Hannes is also right that it would change the behavior of extensions which extend on those library classes (category, content and menu). I can't say however how many extensions would be affected.
The issue is, that the current Joomla router wants to be clever by doing URLs like this: /test/-/-, thus saving the user of the horrid view of IDs for every category on its path. Since the router also does not check the path for anything, it simply checks if the path starts with an ID, then drops the last segment and if that starts with a number, it expects this to be an article view. Long story short: This is fixed in the new router and fixing it in the existing router means quite a rewrite and also a change in behavior. (You couldn't type in whatever you want as category path/alias and still get the same page. While that seems desirable, it has been our behavior so far and thus we can not change it.)
Reminder: We use date-time for Automatic Alias when Title is UTF8 non ascii and Unicode Aliases is not implemented, this not only for menu items.
Thank you for explaining.
If I understand you right, you see no way to solve this problem backwards compatible without changes in the current router. But this will need much more effort. And because of the new router I am not sure if this would be more trouble than it's worth. Perhaps we should investigate this effort in testing the new router. When do you expect the new router ready-made?
Still I think the bug is also not good!
I altered this PR. For solving this issue it is sufficient to do force an alias starting with a letter only for category items. Would this be better for obtaining backwards compatibility?
I did not expect problems with backward compatibility, because I only use the same behavior then the situation of a duplicated alias. Can you tell me a concrete issue?
This PR has received new commits.
CC: @designbengel, @matrikular
As I said earlier, you can not fix this in a backwards compatible way. If you enforce a letter at the beginning, you prevent people from editing their existing sites. If they edit their menu items or categories, all their old URLs would not work anymore, search engine results would point into nirvana, etc. So, no, this is not better for backwards compatibility. You are changing URLs and URLs should never change, especially not unexpected. With this change, people would not associate the change in URLs with the update of Joomla. They would install the new version on their existing site and it could go well for maybe even years. Then one day they change a category and the whole thing blows up in their face and they have no idea why. That this is the result of a change that we forced upon them 3 years ago, is nothing that they can find out.
This PR has received new commits.
CC: @designbengel, @matrikular
OK, now it is clear for me. My changes in this PR are backwards compatible with Joomla, but these changes would force the user to rename existing category name in the case of an edit. And these renaming will change the url ….
So I changed my proposal. The saving of a category with a name starting with a number is now possible. But the user sees a notice, that this name is no good name.
What do you think about this idea?
This PR has received new commits.
CC: @designbengel, @matrikular
This PR has received new commits.
CC: @designbengel, @matrikular
@astridx Thanks for your work on this. I am not sure we should show a warning if a user chooses to save the category starting with a number. The problem of our router shouldn't be pushed into the users faces :) There can be many reasons why users want to save an alias starting with a number. For example a car like a 1960 Cadillac.
I think we should just save the category without a notice and take the issue of a starting number as a known issue.
This PR has received new commits.
CC: @designbengel, @matrikular
My last try :-)
If I understood it right, the main problem with this PR is, that it was not possible to update categories with an alias name starting with a number.
I changed my code in this way that it is now possible to save existing items with an alias name starting with a number. But it is not possible to save new categories with an alias name starting with a number.
And I changed the automatically saving of an alias name that is a date: In the cases an alias name that depends on the date is prefixed with the letter "a"
This PR has received new commits.
CC: @designbengel, @matrikular
The problem is that your PR forces URLs to change and we do not ever allow URLs to change, especially in such a non-obvious way.
Thanks for your answer. But I do not understand why I force URLs to change.
The user has only to change an alias before saving. That is the same as if he wants to use an duplicate alias. Where is my error of thinking?
The alias is part of the URL. If you have a category with alias "2014-cars" now, you are forcing them to change that alias, resulting in a changed URL. We are not talking about new categories, we are talking about existing categories. If someone goes in and just wants to change the category description, he is now also forced to change the alias, which in return changes the URL of the category and all articles inside that category. Simply said, you can not add any new restrictions on the alias of a category (or any alias for that matter), at least not in a minor or patch release. All that said, instead of adding restrictions here, it would be better to fix the router instead.
Hi Hannes,
yes, I understood that this was the problem with the first proposal in this PR.
But yesterday I changed the code.
I only want the users to change an alias name if they create a new item:
(if (preg_match('/^\d/', $this->alias) === 1 && $this->modified_time === $this->created_time))
Users can edit existing items with the new version.
I know that this is not the best solution, but my opinion is that it is better than nothing up to the router works correct :)
The thing is, that you are introducing new behavior and new code without fixing the real problem. At the same time, Joomla tends to not really care what the code is for, but simply duplicates it to everywhere. That is one of the reasons we have the *HelperRoute classes, even though they are wrong on several levels. That is why I would not add this into Joomla.
At the same time, it forces you to change the alias even though there would be no issue. The problem isn't with numeric aliases per se, but with a numeric alias of a child of a child category. So you would have to have a category that is linked via a menu item and then a child category of that and then a child category with a numeric alias of that child category to run into this issue. You however are forcing non-numeric aliases on all new categories.
Regarding checking for new categories only: Please do so based on the existence of an ID or not. The times could be manually changed or non-existing at that time. In any case, they are not a reliable way to determine if a category is new or not.
My opinion is that this solution protects people from this error.
But perhaps you are right and it is not good to implement code that only wants to hush up an error.
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2015-11-14 19:11:20 |
Closed_By | ⇒ | astridx |
I have tested this item successfully on 2529697
Patch solves issue. Tested with the provided instructions for com_category and com_menu.
Note that for a category alias starting with a number, an error type message will be displayed (red) where in com_menu, it is a warning (yellow-ish).
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8141.