Hm... not sure about that. Your suggestion will lead to inconsistent behaviour across models, which imo is worse than the current situation where all models behave wrong but the same until joomla 4.

When we use your suggestion, a plugin that listens to the onContentChangeState event sometimes gets all selected and sometimes only the pks where the state changed depending on the correct usage of column aliases in the corresponding table implementation.

It will also lead to inconsistent ui behavior (different "x items un/published messages) across core/3dp components.

I still prefer #22851 because it uses the table api designed for this use case

When we use your suggestion, a plugin that listens to the onContentChangeState event sometimes gets all selected and sometimes only the pks where the state changed depending on the correct usage of column aliases in the corresponding table implementation.

Since we don't consistently use pre/post events or pass along changesets (as you would be able to extract from an ORM), realistically you're already screwed if you're trying to act only on items that are actually changed.

#22851 is absolutely correct for what it is. Unfortunately as all the PRs post 3.9.1 have shown, the feature is not documented well and used even less, so there is going to have to be a compromise. This is the best compromise that doesn't result in a full revert (i.e. #23197).

I still prefer #22851 because it uses the table api designed for this use case

This PR still uses the same table api ... plus fallback to old behaviour

When we use your suggestion, a plugin that listens to the onContentChangeState event sometimes gets all selected and sometimes only the pks where the state changed depending on the correct usage of column aliases in the corresponding table implementation.

i agree it would be inconsistent,
but is it really much of a problem this inconsistency ?

i would call this inconsistency a fallback behavior
because existing plugins already accept all selected rows, knowing that some of them already had correct state

anyway i do not care much of this PR is merged
i mean we can revert #22851,
just the change that PR #22851 did is really something good

I think about the "inconsistency" (fallback to old behavior)

we can do this
register a JLog message to report that the 'published' column alias has either not been set or it was set to wrong name

this way it will make it easier to have the relevant table classes be patched eventually

Since we don't consistently use pre/post events or pass along changesets (as you would be able to extract from an ORM), realistically you're already screwed if you're trying to act only on items that are actually changed.

sry, dont agree: im able to get changesets for most of the core entities with the help of table observers, existing hooks and the changes made in #22851

And even if i was not able to da that, the fact that the plugin/hook system is inconsistent cant be a reason to introduce more inconsistencies.

The changes proposed in this pr will lead to inconsistent behaviour in both ui and the plugin hooks. Plugins cant rely on the passed data. For me this is not a good compromise.

There are always going to be inconsistencies, there's no getting around it. If a child model overrides this library method and changes the data passed into the event, that affects things just as easily as this perceived core inconsistency. Also, the fact we don't use Event objects with properly declared signatures in the dispatching API (something that's actually fixed under the hood in 4.0) means there is no guarantee on what data is going into plugins in the first place. It's how you end up with stupid bugs about an event in one context being dispatched with a stdClass object and in another context being dispatched with an array.

I still think this is the best course of action and the only alternative is full revert for what I feel is a weak argument.

sry, dont agree: im able to get changesets for most of the core entities with the help of table observers, existing hooks and the changes made in #22851

It's a backhanded way to do it. I was thinking more of Doctrine's UnitOfWork and its pre/post events that include an explicit changeset or methods like isDirty in Laravel's Eloquent models.

Still not very sure if this compromise helps us a lot, but im ok with it for 3.9 as long as we dont forget to revert it in j4 to the way it was before:

• check and fix core tables with table aliases (already done by @JoomliC)
• Add getColumnAlias to table interface (j4)
• Revert change made in this pr (j4)

Still not very sure if this compromise helps us a lot, but im ok with it for 3.9 as long as we dont forget to revert it in j4 to the way it was before:

All of that seems fair.

I have tested this item ✅ successfully on be3a21e

My question why is it now be referenced to 'published' when everything uses 'state' as the column?

Because of this change, I have to fix 5 components on over 50 sites.

My question why is it now be referenced to 'published' when everything uses 'state' as the column?
Because of this change, I have to fix 5 components on over 50 sites.

The above is not so much related to this PR, as the question is related
to the existence of column aliases in (J)Table
and to the existence of method Table::getColumnAlias()

but to answer your question

• the PR that introduced the issue and this PR do not hard-code anything to 'published' column name instead they try to get the alias of it
• you can not change getColumnAlias('published') to getColumnAlias('state') as everywhere else the code is doing setColumnAlias('published', ...) / getColumnAlias('published')` so it would not work furthermore there are important Tables that use 'published' like
  the categories table, the modules table and many 3rd party etc that use "published" so the issue will again be the same

But we could make 1 more "smart" change and check for existence of 'state' column

$publishedColumnName = $table->getColumnAlias('published');

if (property_exists($table, $publishedColumnName) && $table->get($publishedColumnName, $value) == $value)
{
	unset($pks[$i]);
	continue;
}
if (property_exists($table, 'state') && $table->get('state', $value) == $value)
{
	unset($pks[$i]);
	continue;
}

But to start checking the 'state' column this way

• it will be a real bigger inconsistency, than the inconsistency this PR already introduces,
  because everywhere else there is not such check
• and also in (very ??) rare cases it will create a bug , if you have a table that use e.g. an "somename" column as 'published' and a "state" column for something else ...

So i would say just keep the fix made by PR
and add no such checks for state column
or we can just reverse the original PR,
this PR is not important to own extensions anyway
just made an effort to keep the benefit of original PR while making a fix

I have tested this item ✅ successfully on be3a21e

RTC.

@mbabker

can go in stable ?

RTC.

@mbabker

can go in stable ?

But we could make 1 more "smart" change and check for existence of 'state' column

No "smart" fixes. It will break com_contact and any other component where the table being processed has a 'state' column that is not related to publishing state (in this case 'state' is 'province', as in physical address data).

These changes to include column alias is not even in the code on the Joomla documentation on how to build a component. https://docs.joomla.org/J3.x:Developing_an_MVC_Component/Using_the_database

How would any developer find the required changes to their components with reading through Pull requests?

These changes to include column alias is not even in the code on the Joomla documentation on how to build a component. https://docs.joomla.org/J3.x:Developing_an_MVC_Component/Using_the_database

How would any developer find the required changes to their components with reading through Pull requests?

@hrtrulz this PR will fix behavior for third party extensions not using "published" as column name.
So, for those extensions, the prior-to-3.9.1 behavior will apply.
You have to add columnAlias to get advantage of the improvement of this PR.
About ColumnAlias doc: https://docs.joomla.org/Column_alias

True that documentation could be improved at this point! (as column alias introduced in Joomla 3.4...)
Don't forget that everyone can contribute to documentation, as this one is written by the community (very useful, but not prefect, and everyone could help to make it greater!) ;-)

@hrtrulz just updated the page https://docs.joomla.org/J3.x:Developing_an_MVC_Component/Using_the_database to add information about "published" and setColumnAlias()

Often you will find that the database table has a field to keep track of the published/unpublished state of an item. Using the name 'state' within Joomla is not recommended as it can lead to conflicts, instead the name 'published' is used.
Note: How to tell Joomla to store the value of the published form field into a different name database field? We do this by using the method setColumnAlias() (since 3.4.0).

With link to Column Alias docs page.

@JoomliC thanks for updating JDocs.

@JoomliC thanks for updating JDocs.

First docs edit... ;-)
...but in the same time, all component documentation is outdated, and repository for com_helloworld archived... Should we open a new one, and update accordingly the documentation?...

@JoomliC i wrote your Question to Sandra Decoux, her Answer: "Robbie Jackson is updating the MVC tutorial but of course if Cyril can help updating it, it would be great".

Robbie Jackson

Well, if there's a public repository, of course i could help. ;-)