[#47886] - [5.4] [AI] Fix CheckinModel listing rows with checked_out=0 as needing check-in
- Pending
- Medium
- Build: 5.4-dev
- # 47886
- Diff
- genr8r:fix/checkin-list-rows-with-checked-out-zero
User tests: Successful: Unsuccessful:
Hi all,
Hit this on a production Joomla 5.4.6 site while trying to clean up a stuck check-in state. Tracing it back showed the bug isn't environment-specific, it's a predicate mismatch in CheckinModel that any site can hit once a row has been edited and released via the standard Table API.
Pull Request resolves # none (bug not previously reported on the issue tracker).
- I read the Generative AI policy and my contribution is either not created with the help of AI or is compatible with the policy and GNU/GPL 2 or later.
Summary of Changes
CheckinModel::checkin() and CheckinModel::getItems() both use WHERE checked_out IS NOT NULL for nullable checked_out columns. By Joomla's own convention, checked_out = 0 means "not locked": Joomla\CMS\Table\Table::checkIn() explicitly sets $this->checked_out = 0 to release a lock, and Table::isCheckedOut() tests > 0. The IS NOT NULL predicate matches 0, so every row that has been edited and properly released via the API gets counted as needing check-in.
This PR replaces the IS NOT NULL clause with > 0 in both methods. > 0 is correct for nullable columns too because NULL > 0 evaluates to NULL, which WHERE treats as false. The change is two identical hunks; the if/else branches on column nullability become redundant and collapse to a single line.
Testing Instructions
- Pick any table that has a nullable
checked_outcolumn (e.g.#__modules,#__menu,#__ats_cannedrepliesif you have Akeeba ATS installed, or any third-party table that defaultschecked_outto 0). - Set up the "released-via-API" state on one row via SQL:
UPDATE #__modules SET checked_out = 0, checked_out_time = NULL WHERE id = <some-id>;. (You can also produce this state naturally by editing the row in the admin and saving it, which causes Joomla to callTable::checkIn()and store0.) - Go to System → Maintenance → Global Check-in.
Without this PR: the table appears in the list with a count of 1, ticking it and clicking Check-in shows a "success" banner with the raw key text COM_CHECKIN_N_ITEMS_CHECKED_IN, and the row stays in the same state.
With this PR: the table does not appear in the list. To verify the working path still works, set checked_out = 1 (or any value > 0) on a row; the table will appear, ticking and clicking Check-in shows "Item checked in." and clears the lock.
I also confirmed multi-row counts and pluralization still render correctly by locking three rows with checked_out > 0 and checking them in (banner shows "3 items checked in.").
Actual result BEFORE applying this Pull Request
Global Check-in lists tables with a count of N when N rows have checked_out IS NOT NULL, including rows where checked_out = 0 (which are not locked, by Joomla's own convention). Clicking Check-in on such a table runs an UPDATE that matches the same rows but sets checked_out = DEFAULT (already 0) and checked_out_time = NULL (already NULL), so getAffectedRows() returns 0. Text::plural('COM_CHECKIN_N_ITEMS_CHECKED_IN', 0) has no matching _0 variant and no base fallback in com_checkin.ini, so the success banner renders the raw key string.
Expected result AFTER applying this Pull Request
Tables only appear in Global Check-in when at least one row has checked_out > 0 (i.e. is actually locked by a user). Checking those tables in clears the lock and the success banner reads "Item checked in." or "N items checked in." as appropriate. Tables whose rows are all in the released-via-API state (checked_out = 0 or NULL) no longer appear at all, which is the correct behavior because there is nothing to check in.
Link to documentations
Please select:
-
Documentation link for guide.joomla.org:
-
No documentation changes for guide.joomla.org needed
-
Pull Request link for manual.joomla.org:
-
No documentation changes for manual.joomla.org needed
Environment where reproduced
- Joomla: 5.4.6 (production site where bug surfaced, then again on a clean 5.4.6 install for fix verification)
- PHP: 8.1.34
- Database: MySQL 8.0 / MariaDB 10.6 (both reproduce, query semantics identical)
Related
- Issue: none filed (bug not previously on the tracker)
- Upstream status at time of fix: still-broken on
5.4-dev@83d4bf7afe. Identical lines are present on6.1-dev,6.2-dev, and7.0-dev; happy to forward-port or leave that to maintainer up-merge per project convention.
Thanks for taking the time to review. Happy to adjust anything you'd like changed.
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Administration com_checkin |
@richard67 Thanks for the careful read.
On portability: this is ANSI SQL three-valued logic. NULL > 0 evaluates to UNKNOWN, and WHERE treats UNKNOWN as not-true, so the row is filtered. MySQL, MariaDB, and PostgreSQL all behave this way consistently (SQL-92 behavior, predates all three).
On the underlying convention in core: the "both NULL and 0 mean released" semantic is set by libraries/src/Table/Table.php on 5.4-dev @ 83d4bf7afe:
-
Table::checkIn()writes eitherNULLor0depending on$_supportNullValue(Table.php#L1286-L1291):$nullID = $this->_supportNullValue ? 'NULL' : '0'; ... ->set($db->quoteName($checkedOutField) . ' = ' . $nullID)
-
Table::isCheckedOut()treats both as "not locked" via PHP truthy test (Table.php#L1450):if (!$against || ($against == $with)) { return false; }
!0and!nullboth short-circuit to "not checked out".
So both states (NULL and 0) genuinely occur on this column after a clean release, and need to be filtered out either way. checked_out > 0 does that in one predicate; IS NOT NULL AND > 0 does it in two with identical SQL semantics.
On the readability of the two-condition form: my hesitation is that the redundant predicate reads as authorial uncertainty. A reader who doesn't know the NULL semantics sees both conditions and asks "why both?", which is the same knowledge gap the suggestion was meant to close. It moves the confusion rather than removing it.
If reader comprehension is the underlying concern, I'd happily add a short inline comment instead:
// checked_out > 0 also excludes NULL rows (NULL > 0 is UNKNOWN, treated as false by WHERE),
// matching Table::checkIn() which writes either NULL or 0 to release a lock.
$query->where($db->quoteName('checked_out') . ' > 0');That keeps the predicate clean while making both the NULL behavior and the cross-reference explicit. Let me know which way you'd prefer.
| Labels |
Added:
PR-5.4-dev
|
||
| Title |
|
||||||
@richard67 Good point on the style. Switched to the block form and pushed it:
/*
* checked_out > 0 excludes both release states Table::checkIn() can write:
* NULL (when $_supportNullValue) and 0 (otherwise).
* NULL > 0 evaluates to UNKNOWN, which WHERE treats as false.
*/
$query->where($db->quoteName('checked_out') . ' > 0');
@genr8r Can we be sure that it works this way on all supported databases (MySQL, MariaDB and PostgreSQL)?
And even if it does, it is hardly understandable for a code reader who is not an SQL expert.
For better readability and to make sure it always works like we want, I would suggest:
That will result in
That works the same way, and I don't really expect a performance issue with the additional
IS NOT NULLcondition.That's just an idea or suggestion, not a change request by me.
What do you think about it?