Skip to content

Create Article menu item: displays error if Category is not selected when Default Category is set#25798

Merged
HLeithner merged 4 commits intojoomla:stagingfrom
infograf768:create_article_error
Aug 27, 2019
Merged

Create Article menu item: displays error if Category is not selected when Default Category is set#25798
HLeithner merged 4 commits intojoomla:stagingfrom
infograf768:create_article_error

Conversation

@infograf768
Copy link
Member

@infograf768 infograf768 commented Aug 9, 2019

Pull Request for Issue #17708

Summary of Changes

as title says

Testing Instructions

Create a new menu item of type "Create Article"
In the Options tab, set Default Category to Yes
Do NOT select a category in the field below which only displays by a showon.
Save menu item.

Before patch

The menu item is saved. No error is set.
Category id was wrongly set to 1, which wrong behaviour is now corrected in frontend, among other issues with permissions, with #17674

After patch

Preventing saving and displaying an error message

Screen Shot 2019-08-09 at 09 50 28

@alikon @LivioCavallo

@brianteeman
Copy link
Contributor

  1. I'm assuming that the obvious option of making the field required is not used for a valid reason

  2. COM_CONTENT_CREATE_ARTICLE_ERROR="When default category is enabled, a category should be selected."
    As its an error message not a help doc this can be rewritten to something like
    COM_CONTENT_CREATE_ARTICLE_ERROR="A default category is required."

@infograf768
Copy link
Member Author

I'm assuming that the obvious option of making the field required is not used for a valid reason

It's impossible as the field should be ignored when Default category is set to no and we do not have a specific "requiredon" (on the model of "showon" code).

COM_CONTENT_CREATE_ARTICLE_ERROR="When default category is enabled, a category should be selected."
As its an error message not a help doc this can be rewritten to something like
COM_CONTENT_CREATE_ARTICLE_ERROR="A default category is required."

Why not give this detail? It does not cost much and help user solve the issue. grrr

@ghost
Copy link

ghost commented Aug 9, 2019

I have tested this item ✅ successfully on 0a9638a


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

@richard67
Copy link
Member

I have tested this item ✅ successfully on 261fdbe


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

@richard67
Copy link
Member

@franz-wohlkoenig Last change after your test was just a language change, so you can just set your test result again without retest, just review last change, and then RTC 😄

@infograf768
Copy link
Member Author

Don't worry about appveyor btw.

@richard67
Copy link
Member

I know.

@ghost
Copy link

ghost commented Aug 9, 2019

Status "Ready To Commit".

@HLeithner
Copy link
Member

Sorry to say but this solution is really hacky in my opinion that should be done in a plugin for example content->joomla in the on_before_save event.

@infograf768
Copy link
Member Author

@HLeithner
I would not know how to do what you suggest.

@richard67
Copy link
Member

richard67 commented Aug 24, 2019

@HLeithner I would also not know. I have an idea what you mean but I'm not good enough in event based stff to do it either. And it has 2 good tests, one from me. And I would not call it "hacky", only maybe "old style but working". So for me it's pretty fine.

P.S.: And it fixes an issue.

@HLeithner
Copy link
Member

@richard67 writing a hardcoded string into com_menu because a specific other component needs it is wrong. It's possible that we don't have another way but if we have the choice we should use it.

Please but this in to plg_content_joomla + the language string.

	/**
	 * The save event.
	 *
	 * @param   string   $context  The context
	 * @param   object   $table    The item
	 * @param   boolean  $isNew    Is new item
	 * @param   array    $data     The validated data
	 *
	 * @return  void
	 *
	 * @since   __DEPLOY_VERSION__
	 */
	public function onContentBeforeSave($context, $table, $isNew, $data)
	{
		// Check we are handling the frontend edit form.
		if ($context !== 'com_menus.item')
		{
			return true;
		}

		// Special case for Create article menu item
		if ($table->link !== 'index.php?option=com_content&view=form&layout=edit')
		{
			return true;
		}

		// Display error if catid is not set when enable_category is enabled
		$params = json_decode($table->params, true);

		if ($params['enable_category'] == 1 && empty($params['catid']))
		{
			$table->setError(JText::_('PLG_CONTENT_JOOMLA_CREATE_ARTICLE_WITH_EMPTY_CATEGORY_ERROR'));
			return false;
		}
	}

@richard67
Copy link
Member

@HLeithner Is not my PR, is @infograf768

@infograf768
Copy link
Member Author

will test. No idea what happens btw in general when that plugin is disabled.

@infograf768 infograf768 force-pushed the create_article_error branch from 5b67f44 to 961b9cd Compare August 26, 2019 16:55
@infograf768
Copy link
Member Author

@richard67 @HLeithner
used plugin instead. Please test.
(The lang string has to remain in com_content.ini as that plugin is not loading its lang files in the page concerned.)

@richard67
Copy link
Member

I have tested this item ✅ successfully on b04c352


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

@richard67
Copy link
Member

@franz-wohlkoenig Could you test again?

@ghost
Copy link

ghost commented Aug 26, 2019

I have tested this item ✅ successfully on b04c352


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

@HLeithner
Copy link
Member

Thanks for catching this error.

@HLeithner HLeithner merged commit 16eb98e into joomla:staging Aug 27, 2019
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Aug 27, 2019
@HLeithner HLeithner added this to the Joomla! 3.9.12 milestone Aug 27, 2019
@infograf768 infograf768 deleted the create_article_error branch August 27, 2019 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Language Change This is for Translators

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants