Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix tags being deleted by any batch action and by drap and drop reordering of records #18328

Merged
merged 10 commits into from
Oct 23, 2017
Merged

Fix tags being deleted by any batch action and by drap and drop reordering of records #18328

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
caf8b82
Fix tag not retrieved properly due to wrong className and wrong typeA…
ggppdk Oct 13, 2017
9f7f7de
Removed double colon
ggppdk Oct 13, 2017
5fada9c
Fixed duplicate word in comment
ggppdk Oct 13, 2017
d05af41
Document 3 new class variables, and also update batchTag to use them
ggppdk Oct 15, 2017
b342d4a
Fix not defined local variable 'user' in last commit
ggppdk Oct 15, 2017
0b633ca
Fix not defined local variable 'batchSet'
ggppdk Oct 15, 2017
8a48d5f
Better comments
ggppdk Oct 15, 2017
1a9524c
Convert static variable to member variable, better comments
ggppdk Oct 16, 2017
331582f
Restore variable names and make them member properties again to have B/C
ggppdk Oct 16, 2017
88fec66
Restore 1 more local variable as a member property
ggppdk Oct 17, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
129 changes: 58 additions & 71 deletions libraries/src/MVC/Model/AdminModel.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,21 +209,8 @@ public function batch($commands, $pks, $contexts)

$done = false;

// Set some needed variables.
$this->user = \JFactory::getUser();
$this->table = $this->getTable();
$this->tableClassName = get_class($this->table);
$this->contentType = new \JUcmType;
$this->type = $this->contentType->getTypeByTable($this->tableClassName);
$this->batchSet = true;

if ($this->type == false)
{
$type = new \JUcmType;
$this->type = $type->getTypeByAlias($this->typeAlias);
}

$this->tagsObserver = $this->table->getObserverOfClass('\JTableObserverTags');
// Initialize needed member properties
$this->initBatch();

if ($this->batch_copymove && !empty($commands[$this->batch_copymove]))
{
Expand Down Expand Up @@ -293,15 +280,8 @@ public function batch($commands, $pks, $contexts)
*/
protected function batchAccess($value, $pks, $contexts)
{
if (empty($this->batchSet))
{
// Set some needed variables.
$this->user = \JFactory::getUser();
$this->table = $this->getTable();
$this->tableClassName = get_class($this->table);
$this->contentType = new \JUcmType;
$this->type = $this->contentType->getTypeByTable($this->tableClassName);
}
// Initialize needed member properties
$this->initBatch();

foreach ($pks as $pk)
{
Expand Down Expand Up @@ -350,15 +330,8 @@ protected function batchAccess($value, $pks, $contexts)
*/
protected function batchCopy($value, $pks, $contexts)
{
if (empty($this->batchSet))
{
// Set some needed variables.
$this->user = \JFactory::getUser();
$this->table = $this->getTable();
$this->tableClassName = get_class($this->table);
$this->contentType = new \JUcmType;
$this->type = $this->contentType->getTypeByTable($this->tableClassName);
}
// Initialize needed member properties
$this->initBatch();

$categoryId = $value;

Expand Down Expand Up @@ -470,15 +443,8 @@ protected function batchCopy($value, $pks, $contexts)
*/
protected function batchLanguage($value, $pks, $contexts)
{
if (empty($this->batchSet))
{
// Set some needed variables.
$this->user = \JFactory::getUser();
$this->table = $this->getTable();
$this->tableClassName = get_class($this->table);
$this->contentType = new \JUcmType;
$this->type = $this->contentType->getTypeByTable($this->tableClassName);
}
// Initialize needed member properties
$this->initBatch();

foreach ($pks as $pk)
{
Expand Down Expand Up @@ -527,15 +493,8 @@ protected function batchLanguage($value, $pks, $contexts)
*/
protected function batchMove($value, $pks, $contexts)
{
if (empty($this->batchSet))
{
// Set some needed variables.
$this->user = \JFactory::getUser();
$this->table = $this->getTable();
$this->tableClassName = get_class($this->table);
$this->contentType = new \JUcmType;
$this->type = $this->contentType->getTypeByTable($this->tableClassName);
}
// Initialize needed member properties
$this->initBatch();

$categoryId = (int) $value;

Expand Down Expand Up @@ -631,7 +590,7 @@ protected function batchTag($value, $pks, $contexts)
/**
* @var \JTableObserverTags $tagsObserver
*/
$tagsObserver = $table->getObserverOfClass('\JTableObserverTags');
$tagsObserver = $table->getObserverOfClass('Joomla\CMS\Table\Observer\Tags');;
Copy link
Contributor

Choose a reason for hiding this comment

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

remove double semicolon

$result = $tagsObserver->setNewTags($tags, false);

if (!$result)
Expand Down Expand Up @@ -1307,50 +1266,48 @@ public function save($data)
*/
public function saveorder($pks = array(), $order = null)
{
$table = $this->getTable();
$tableClassName = get_class($table);
$contentType = new \JUcmType;
$type = $contentType->getTypeByTable($tableClassName);
$tagsObserver = $table->getObserverOfClass('\JTableObserverTags');
// Initialize needed member properties
$this->initBatch();

$conditions = array();

if (empty($pks))
{
return \JError::raiseWarning(500, \JText::_($this->text_prefix . '_ERROR_NO_ITEMS_SELECTED'));
}

$orderingField = $table->getColumnAlias('ordering');
$orderingField = $this->table->getColumnAlias('ordering');

// Update ordering values
foreach ($pks as $i => $pk)
{
$table->load((int) $pk);
$this->table->load((int) $pk);

// Access checks.
if (!$this->canEditState($table))
if (!$this->canEditState($this->table))
{
// Prune items that you can't change.
unset($pks[$i]);
\JLog::add(\JText::_('JLIB_APPLICATION_ERROR_EDITSTATE_NOT_PERMITTED'), \JLog::WARNING, 'jerror');
}
elseif ($table->$orderingField != $order[$i])
elseif ($this->table->$orderingField != $order[$i])
{
$table->$orderingField = $order[$i];
$this->table->$orderingField = $order[$i];

if ($type)
if ($this->type)
{
$this->createTagsHelper($tagsObserver, $type, $pk, $type->type_alias, $table);
$this->createTagsHelper($this->tagsObserver, $this->type, $pk, $this->typeAlias, $this->table);
}

if (!$table->store())
if (!$this->table->store())
{
$this->setError($table->getError());
$this->setError($this->table->getError());

return false;
}

// Remember to reorder within position and client_id
$condition = $this->getReorderConditions($table);
$condition = $this->getReorderConditions($this->table);
$found = false;

foreach ($conditions as $cond)
Expand All @@ -1364,17 +1321,17 @@ public function saveorder($pks = array(), $order = null)

if (!$found)
{
$key = $table->getKeyName();
$conditions[] = array($table->$key, $condition);
$key = $this->table->getKeyName();
$conditions[] = array($this->table->$key, $condition);
}
}
}

// Execute reorder for each category.
foreach ($conditions as $cond)
{
$table->load($cond[0]);
$table->reorder($cond[1]);
$this->table->load($cond[0]);
$this->table->reorder($cond[1]);
}

// Clear the component's cache
Expand Down Expand Up @@ -1478,4 +1435,34 @@ public function generateTitle($categoryId, $table)
$table->title = $data['0'];
$table->alias = $data['1'];
}

/**
* Method to initialize member variables used by batch methods and other methods like saveorder()
*
* @return void
*
* @since __DEPLOY_VERSION__
*/
public function initBatch()
{
if (empty($this->batchSet))
{
$this->batchSet = true;
$this->user = \JFactory::getUser();
Copy link
Contributor

Choose a reason for hiding this comment

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

New class member variables should be documented for this and other $this->foo assignments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, but the previous guy that originally added them, did not do it,

you mean add them at the top of the class, together with other member variables,
and document them similarly ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I ll document them later today,
also 2 of them are not really needed to be defined as member variables


// Get table and table class
$this->table = $this->getTable();
$this->tableClassName = get_class($this->table);
$tc = explode('\\', $this->tableClassName);
$this->tableClassName = end($tc);

// Get content Type
$this->contentType = new \JUcmType;
$this->type = $this->contentType->getTypeByTable($this->tableClassName)
?: $this->contentType->getTypeByAlias($this->typeAlias);

Copy link
Contributor

Choose a reason for hiding this comment

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

To be strict I suggest to initialise $this->contentType as before.

It would be nice to deprecate a few class variables (contentType, tableClassName, user) that you do not need right now if everyone agrees.

// Get get tabs observer
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove duplicate get

$this->tagsObserver = $this->table->getObserverOfClass('Joomla\CMS\Table\Observer\Tags');
}
}
}