Skip to content

Conversation

@SharkyKZ
Copy link
Contributor

@SharkyKZ SharkyKZ commented May 28, 2019

Pull Request for Issue #22216.

Summary of Changes

Adds prefix to categories created on the fly to allow creating categories starting with numbers.

Testing Instructions

  1. Create a category and remember the ID (or use an existing category) e.g. "2"
  2. Create a new article or edit one and try to create a new category with the name: "2nd workshop" (important: the ID of the category from 1. has to be the leading character) by typing in the name in the category select on the right side + confirm with [ENTER]
  3. Save article

Expected result

A new category is created and assigned to the article

Actual result

The old category from 1 is assigned

Documentation Changes Required

New Categoryedit field property added.
Categories created on the fly contain #new# prefix.

@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-staging labels May 28, 2019
@SharkyKZ
Copy link
Contributor Author

On second thought this could be a B/C break for those programatically adding articles using the model. If using category title, it must now contain #new# prefix or else saving fails. Thoughts?

@joomla-cms-bot joomla-cms-bot removed the Language Change This is for Translators label May 28, 2019
@SharkyKZ
Copy link
Contributor Author

Should be OK now.

@ghost ghost changed the title Creating categories on the fly with numbers. Creating categories on the fly with numbers Jun 15, 2019
@ReLater
Copy link
Contributor

ReLater commented Jun 15, 2019

I have tested this item ✅ successfully on 4cf418c


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

1 similar comment
@Quy
Copy link
Contributor

Quy commented Jun 15, 2019

I have tested this item ✅ successfully on 4cf418c


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

@Quy
Copy link
Contributor

Quy commented Jun 15, 2019

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jun 15, 2019
@HLeithner
Copy link
Member

@bembelimen can you confirm that this fix your problem?


// Save New Category
if ($catid == 0 && $this->canCreateCategory())
if ($createCategory && $this->canCreateCategory())
Copy link
Contributor

@bembelimen bembelimen Jul 20, 2019

Choose a reason for hiding this comment

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

In general the PR solves the problem, thank you @SharkyKZ very good work.
I have one general thought here (which I'm not sure if related to this PR or needs a new one):

Let's assume we transfer a string as catid (so $createCategory is true) but $this->canCreateCategory() is false, what is the saved value for catid then? I assume "0"?

So tltr: PR is fine imho and can be merged. If this special (edge) case would also be handled, it would be nice, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The UI wouldn't allow to create category, but if you circumvent it, the ID would be 0. It's the same before this PR though.

@HLeithner HLeithner merged commit 4a901a1 into joomla:staging Jul 22, 2019
@HLeithner
Copy link
Member

thx

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jul 22, 2019
@HLeithner HLeithner added this to the Joomla! 3.9.11 milestone Jul 22, 2019
@wilsonge
Copy link
Contributor

Sorry I just came across this. Isn't this a b/c break for people using com_categories because they need to change their models (like we had to in com_content). I think the component specific ones in banners and contact is fine) but not the com_categories one which is generic

@SharkyKZ
Copy link
Contributor Author

@wilsonge No, the custom prefix property added to JFormFieldCategoryEdit is optional.

@wilsonge
Copy link
Contributor

OK makes sense - missed that - thanks!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants