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.

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

[FIX 3.9.1] Add column alias com_finder Search Filter (Published state broken) #23194

Merged
merged 1 commit into from Dec 1, 2018

Conversation

Projects
None yet
8 participants
@JoomliC
Copy link
Contributor

JoomliC commented Nov 28, 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
@Quy

This comment has been minimized.

Copy link
Contributor

Quy commented Nov 28, 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.

1 similar comment
@infograf768

This comment has been minimized.

Copy link
Member

infograf768 commented Nov 29, 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.

@Yiannistaos

This comment has been minimized.

Copy link

Yiannistaos commented on a12ba33 Nov 29, 2018

It works! Thanks.
But what about dozens of custom components?

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)

This comment has been minimized.

Copy link
Member

infograf768 replied Nov 29, 2018

Not sure this would always work.
The __extensions table has
$this->setColumnAlias('published', 'enabled');

and not state

This comment has been minimized.

Copy link

Yiannistaos replied Nov 29, 2018

Not sure this would always work.
The __extensions table has
$this->setColumnAlias('published', 'enabled');

and not state

correct!

@laoneo

This comment has been minimized.

Copy link
Member

laoneo commented Nov 29, 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?

@alikon

This comment has been minimized.

Copy link
Contributor

alikon commented Nov 29, 2018

maybe #22851 have side-effect

@laoneo

This comment has been minimized.

Copy link
Member

laoneo commented Nov 29, 2018

That's the pr which broke it.

@JoomliC

This comment has been minimized.

Copy link
Contributor Author

JoomliC commented Nov 29, 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)

@JoomliC

This comment has been minimized.

Copy link
Contributor Author

JoomliC commented Nov 29, 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)

@JoomliC

This comment has been minimized.

Copy link
Contributor Author

JoomliC commented Nov 29, 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...

@laoneo

This comment has been minimized.

Copy link
Member

laoneo commented Nov 29, 2018

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

@laoneo

This comment has been minimized.

Copy link
Member

laoneo commented Nov 29, 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.

@mbabker

This comment has been minimized.

Copy link
Member

mbabker commented Nov 29, 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.

@JoomliC

This comment has been minimized.

Copy link
Contributor Author

JoomliC commented Nov 29, 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

@JoomliC

This comment has been minimized.

Copy link
Contributor Author

JoomliC commented Nov 29, 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 ?

@mbabker

This comment has been minimized.

Copy link
Member

mbabker commented Nov 29, 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.

@JoomliC

This comment has been minimized.

Copy link
Contributor Author

JoomliC commented Nov 29, 2018

👍
Make sense!

@Quy

This comment has been minimized.

Copy link
Contributor

Quy commented Nov 29, 2018

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC label Nov 29, 2018

@mbabker mbabker added this to the Joomla 3.9.2 milestone Dec 1, 2018

@mbabker mbabker merged commit dc19d12 into joomla:staging Dec 1, 2018

4 checks passed

JTracker/HumanTestResults Human Test Results: 2 Successful 0 Failed.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@joomla-cms-bot joomla-cms-bot added PR-staging and removed RTC labels Dec 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment