User tests: Successful: Unsuccessful:
Pull Request for Issue with com_finder Smart Search Filters published state broken in 3.9.1
Related to PR #22851
Status | New | ⇒ | Pending |
Category | ⇒ | Administration com_finder |
I have tested this item
I have tested this item
Honestly I'm wondering if we do not have here BC break. As unpublishing is also not working anymore in DPCalendar. Should we not fix the cause instead?
That's the pr which broke it.
Yes, #22851 is a B/C break if you consider that it changed a previously working behavior. This was my first thought, and in my extension too i had this issue.
But the fact is this changed is logical, and in other places of Joomla core, some other tables were already declaring an alias for published column (com_fields for example).
We can fix B/C here by adding a state
check as well for column alias but what would happen if a table use state
column as an address part ?
Maybe the way would be to define a standard for "published" as a column name when using Joomla api for status control (we already have standard id
, checked_out
, published_up
...)
The problem today is Joomla core using mainly published
, and sometimes state
for the same function.
This maybe should be solved, by using only one naming convention, to be used properly by api.
eg. Batch copy checks for published column first, and then for state column (https://github.com/joomla/joomla-cms/blob/staging/libraries/src/MVC/Model/AdminModel.php#L444). But what if a table has no status system, but a list of locations, and a batch copy function (this may fail if state column exists for postal state)
Now, to give my personal opinion: i used state
as status column as it was used elsewhere in Joomla core, and for me, more sense than published
(because more than just a published/unpublished state).
i would prefer state as standard, but that means to have a reserved list of column names for Joomla api usage. (the reserved name should not be used for anything else than what the api needs it)
Do you think that it would be better to edit the core file "\libraries\src\MVC\Model\AdminModel.php", at line 1066. It worked for my custom components.
FROM:
if ($table->get($table->getColumnAlias('published'), $value) == $value)
TO:
if ($table->get($table->getColumnAlias('state'), $value) == $value)
I think best (if purpose to fix a B/C break) would be an elseif
checking first if published
column exists. If not, checking the state
column.
The problem is Joomla core using 3 naming (published, state, enabled) for the same functionnality.
There is the mistake for consistency, and for code information returned to third party developers...
But then we have to fix this in joomla 4 and not in a patch release as it is a BC break.
Before testing this one, better to wait for a decision made in #23197.
No, this should still be tested and included in core regardless of #23197. The fact that these aliases have not been included into the core components is a bug in their own right.
Maybe the way would be to define a standard for "published" as a column name when using Joomla api for status control (we already have standard
id
,checked_out
,published_up
...)
We really should not require explicit column names. That is exactly what this aliasing logic is supposed to take care of. In the context of com_contact, "state" refers to what most people consider a "province" and not an alias of "published" so you can't place a hard reservation on the "state" column name without creating problems for any extension with a table storing addressing data.
We really should not require explicit column names. That is exactly what this aliasing logic is supposed to take care of. In the context of com_contact, "state" refers to what most people consider a "province" and not an alias of "published" so you can't place a hard reservation on the "state" column name without creating problems for any extension with a table storing addressing data.
So... even published
can't be reserved ?
It can be documented that published
is the preferred way and the default name core expects, but everything in core interacting with that column should be using the table API's aliasing feature and if a table uses a different column name for that feature then it should still work.
We don't force id
to be a table's PK, the API lets you specify the PK for your table (and does support composite PKs so it's not even restricted to a single column). We might default to id
but I don't even think that happens. The same convention applies here for published
or checked_out
or any other feature.
Make sense!
Status | Pending | ⇒ | Ready to Commit |
RTC
Status | Ready to Commit | ⇒ | Fixed in Code Base |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2018-12-01 18:04:04 |
Closed_By | ⇒ | mbabker | |
Labels |
Added:
?
|
I have tested this item✅ successfully on a12ba33
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/23194.