[#33348] fix JModelAdmin::publish() #3190

itbra · 2014-02-26T18:11:41Z

Fixing issue described in bug #33348, related to JModelAdmin (libraries/legacy/model/admin.php)

When trying to publish an item, the related table might be(en) checked out by another user. Thus, publishing will fail silently, since the table is going to try to check in only if is not checked out at all or if the user trying to check it in equals the one having checked it out. If they mismatch nothing is stated to the user trying to change its state, which is no good. So i added a check for this circumstance.

Replaced 'AND' by '&&'

The model assumed the table has a field named 'checked_out'. But this might cause an error being thrown trying to access a non-existing property with tables not having this field.

Added property existence check

Fixed logical error in line 936

zero-24 · 2015-05-03T20:14:34Z

libraries/legacy/model/admin.php

+				// If the table is checked out by another user, drop it and report to the user trying to change its state.
+				if (property_exists($table, 'checked_out') && $table->checked_out && ($table->checked_out != $user->id))
+				{
+					JLog::add(JText::sprintf('COM_KEY2SWAP_ERROR_ROW_IS_CHECKED_OUT_BY', $pks[$i]), JLog::WARNING, 'jerror');


@itbra
COM_KEY2SWAP_ERROR_ROW_IS_CHECKED_OUT_BY
Is not a Core language string can you change it to something that is a core one or add an new core one?

Done.

roland-d · 2015-05-04T19:06:41Z

@itbra Could you please update the PR so it is up-to-date. I did a quick test with manual code change and it works for me and is a good fix. Thanks.

itbra · 2015-05-04T20:25:03Z

I can do that, but i'm afraid it makes no sense as it seems there is no further interest to accept and merge it. See how old it is.

zero-24 · 2015-05-04T20:42:40Z

@itbra I have just also tested as @roland-d per manual code change and it works here too. So the only thing for RTC/merge is now is a up-to-date PR. 😄

roland-d · 2015-05-04T20:46:39Z

Agree, once the PR is up-to-date I will merge it as we have 2 good tests. Thanks @itbra

itbra · 2015-05-04T22:05:06Z

Ok, so i'll close this PR and create a new one as i don't know how to update this one against the latest Joomla! version here on GitHub and creating a new one is the fastest way for me. I create all my PRs directly on GitHub.

itbra · 2015-05-05T10:40:49Z

Guys i need your opinions!

When i started to create the new PR i noticed that JModelAdmin::reorder() and JModelAdmin::saveorder() also change an item's state not considering that it might be checked out by a different user. As i think every change to an item's state while it might be checked out by a different user should be checked for this conflict i wonder if it wouldn't be better to extend JModelAdmin::canEditState() rather than introducing code duplication.

I don't want to create the new PR with duplicate code but would like to update all methods that appear to be buggy like JModelAdmin::publish() regarding the editor mismatch.

What do you think?

roland-d · 2015-05-06T05:46:20Z

@itbra I agree with your comment that the checked out status should be taken into account of the reorder/saveorder actions.

However I think we should fix it a little higher up in the chain. If you look at JTable::published() we have the checked out check there as well. This code:

// Determine if there is checkin support for the table.
if (property_exists($this, 'checked_out') || property_exists($this, 'checked_out_time'))
{
    $query->where('(' . $this->getColumnAlias('checked_out') . ' = 0 OR ' . $this->getColumnAlias('checked_out') . ' = ' . (int) $userId . ')');
    $checkin = true;
}
else
{
    $checkin = false;
}

Not sure if we can/should wrap this into a method of it's own. You are setting 2 values here and it is only a single if/else block. Using this block in the existing methods might be just as effective as you don't have to juggle with parameters and return values.

That is my idea.

RonakParmar · 2016-02-16T09:58:42Z

I have checked this issue in my local, able to reproduce the issue of changing the state of checked item.
Here is my system information:

Joomla! Version: Joomla! 3.4.8 Stable [ Ember ] 24-December-2015 19:30 GMT
Joomla! Platform Version: Joomla Platform 13.1.0 Stable [ Curiosity ] 24-Apr-2013 00:00 GMT
PHP Version: 5.4.45-3+deb.sury.orgprecise+1
System Linux desktop 3.5.0-54-generic #81precise1-Ubuntu SMP Tue Jul 15 04:02:22 UTC 2014 x86_64