? Pending

User tests: Successful: Unsuccessful:

avatar JoomliC
JoomliC
28 Nov 2018

Pull Request for Issue with com_finder Smart Search Filters published state broken in 3.9.1
Related to PR #22851

Summary of Changes

  • Add column alias

Testing Instructions

  • Add Search filters and test to change published state for one or multiple Filters with button and checkbox.
  • Apply this PR and test again.

Expected result

  • Published state is changed
    capture d ecran 2018-11-28 a 17 32 17

Actual result

  • No effect
    capture d ecran 2018-11-28 a 17 32 38
avatar JoomliC JoomliC - open - 28 Nov 2018
avatar JoomliC JoomliC - change - 28 Nov 2018
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 28 Nov 2018
Category Administration com_finder
avatar Quy
Quy - comment - 28 Nov 2018

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.

avatar Quy
Quy - comment - 28 Nov 2018

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.

avatar Quy Quy - test_item - 28 Nov 2018 - Tested successfully
avatar infograf768
infograf768 - comment - 29 Nov 2018

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.

avatar infograf768 infograf768 - test_item - 29 Nov 2018 - Tested successfully
avatar laoneo
laoneo - comment - 29 Nov 2018

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?

avatar alikon
alikon - comment - 29 Nov 2018

maybe #22851 have side-effect

avatar laoneo
laoneo - comment - 29 Nov 2018

That's the pr which broke it.

avatar JoomliC
JoomliC - comment - 29 Nov 2018

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)

avatar JoomliC
JoomliC - comment - 29 Nov 2018

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)

avatar JoomliC
JoomliC - comment - 29 Nov 2018

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...

avatar laoneo
laoneo - comment - 29 Nov 2018

But then we have to fix this in joomla 4 and not in a patch release as it is a BC break.

avatar laoneo
laoneo - comment - 29 Nov 2018

Made a pr #23197 which reverts #22851, better to not break BC in 3 and do it properly in 4. Not sure if it is even an issue on 4 with workflows.

Before testing this one, better to wait for a decision made in #23197.

avatar mbabker
mbabker - comment - 29 Nov 2018

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.

avatar JoomliC
JoomliC - comment - 29 Nov 2018

But then we have to fix this in joomla 4 and not in a patch release as it is a BC break.

I don't fix column naming here (this should be for Joomla 4) but missing column aliases for a proper use of Joomla api "publish" item.

Open an issue RFC for Joomla 4: #23198

avatar JoomliC
JoomliC - comment - 29 Nov 2018

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 ?

avatar mbabker
mbabker - comment - 29 Nov 2018

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.

avatar JoomliC
JoomliC - comment - 29 Nov 2018

👍
Make sense!

avatar Quy Quy - change - 29 Nov 2018
Status Pending Ready to Commit
avatar Quy
Quy - comment - 29 Nov 2018

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/23194.

avatar mbabker mbabker - change - 1 Dec 2018
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: ?
avatar mbabker mbabker - close - 1 Dec 2018
avatar mbabker mbabker - merge - 1 Dec 2018

Add a Comment

Login with GitHub to post a comment