? Success

User tests: Successful: Unsuccessful:

avatar eshiol
eshiol
10 Apr 2018

There's no way of knowing if a user has changed the access level of an article if he uses the batch command

Summary of Changes

Added the events event_before_save and event_after_save to the function batchAccess in AdminModel class

Testing Instructions

Expected result

Actual result

Documentation Changes Required

avatar eshiol eshiol - open - 10 Apr 2018
avatar eshiol eshiol - change - 10 Apr 2018
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 10 Apr 2018
Category Libraries
avatar eshiol eshiol - change - 10 Apr 2018
Labels Added: ?
avatar eshiol
eshiol - comment - 14 Apr 2018

I think this is a very important aspect of security. I wrote a simple plug-in to record user access to sensitive articles, but if a user can change the access level of an article without firing a trigger, the plug-in is not useful.

avatar mbabker
mbabker - comment - 14 Apr 2018

A few things:

  • I'm pretty sure the discussion of firing events on batch actions has come up before in various ways with various results, a search might reveal some context I just don't remember off hand
  • The events need to fire on every batch action, not only access
  • This really needs performance benchmarking, what is going to happen if someone batch updates 20 items, or 50, or even more?
  • This really needs communicated if/when merged, I can almost guarantee with some form of certainty some plugins hooking these events are only expecting them to trigger in the context of a single record update (rightfully so, that's how it goes right now), giving these events new context where they may fire multiple times in one request is probably going to mess with those plugins in some way
avatar laoneo
laoneo - comment - 16 Apr 2018

For edit operations, mainly in the back end, I guess it would be acceptable the performance loss. Read operations on the front end are performance crucial.

But. During development of custom fields, we got into a similar issue. In the list view is for every entity an event fired to gather the display data, eg. custom fields. We were talking there already about batch events to trigger one event with multiple entities. So for me this would be a welcoming addition.

avatar eshiol
eshiol - comment - 16 Apr 2018

We could use onContentBeforeBatch and onContentAfterBatch in the batch function

	/**
	 * Content is passed by reference. Method is called before the batch function is runned.
	 *
	 * @param   array  $commands  An array of commands to perform.
	 * @param   array  $pks       An array of item ids.
	 * @param   array  $contexts  An array of item contexts.
	 *
	 * @return  boolean  Returns true on success, false on failure.
	 *
	 * @since   __DEPLOY_VERSION__
	 */
	public function onContentBeforeBatch(&$commands, &$pks, &$contexts) {}
	
	/**
	 * Method is called after the batch function is runned.
	 *
	 * @param   array  $commands  An array of commands to perform.
	 * @param   array  $pks       An array of item ids.
	 * @param   array  $contexts  An array of item contexts.
	 *
	 * @return  boolean  Returns true on success, false on failure.
	 *
	 * @since   __DEPLOY_VERSION__
	 */
	public function onContentAfterBatch($commands, $pks, $contexts) {}
avatar eshiol
eshiol - comment - 18 Apr 2018

A good compromise, to maintain high performance, could be the use of the events onContentBeforeBatch and onContentAfterBatch for all batch commands except for copy, in that case you could use onContentBeforeSave and onContentAfterSave

	/**
	 * Method is called before the batch function is runned.
	 *
	 * @param   string $identifier  The field to change.
	 * @param   string $command     The command to perform.
	 * @param   array  $pks         An array of item ids.
	 * @param   array  $contexts    An array of item contexts.
	 *
	 * @return  boolean  Returns true on success, false on failure.
	 *
	 * @since   __DEPLOY_VERSION__
	 */
	public function onContentBeforeBatch($identifier, $command, &$pks, &$contexts) {}

	/**
	 * Method is called after the batch function is runned.
	 *
	 * @param   string $identifier  The field to change.
	 * @param   string $command     The command to perform.
	 * @param   array  $pks         An array of item ids.
	 * @param   array  $contexts    An array of item contexts.
	 *
	 * @return  void
	 *
	 * @since   __DEPLOY_VERSION__
	 */
	public function onContentAfterBatch($identifier, $command, $pks, $contexts) {}
avatar ggppdk
ggppdk - comment - 22 Apr 2018

Performance of event triggering in batch operations has troubled me too

I am also in favour of introducing onContentBeforeBatch, onContentAfterBatch
It is a chance to get it right now, before introducing something that later will be a B/C break to undo

avatar eshiol
eshiol - comment - 22 Apr 2018

I added a new PR #20213 for the events onContentBeforeBatch and onContentAfterBatch

avatar ghost
ghost - comment - 21 Jul 2019

@eshiol can you please give test instructions in opening comment?

avatar laoneo
laoneo - comment - 25 Mar 2022

Sorry that it took so long to respond. As this pr can have some performance impact on batch processing I'm closing it favor of #20213. Thank you very much for your contribution making Joomla better.

avatar laoneo laoneo - change - 25 Mar 2022
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2022-03-25 08:17:38
Closed_By laoneo
Labels Added: ?
Removed: ?
avatar laoneo laoneo - close - 25 Mar 2022

Add a Comment

Login with GitHub to post a comment