Revert "[Batch] Change State: pass only affected items to table

[laoneo]

Pull Request for #23194, #23193, #23192, #23191 and probably dozens of support requests on 3rd party extensions where unpublish doesn't work anymore in the back end.

Summary of Changes

#22851 fixed bulk state changes but in a none BC way. After talking with @dneukirchen it is better to revert this change and apply it on joomla 4 as it is clearly a BC break.

Testing Instructions

• Open banners in the back end
• Create a new client and click save and close
• On the client list, click on the publish button beside the new client

Expected result

Client is unpublished.

Actual result

Client is still published.

[mbabker]

apply it on joomla 4 as it is clearly a BC break

Actually, no, it is not "clearly a BC break". Everything about #22851 is just fine minus the fact that nothing uses the column aliasing logic in the Table API even though it has been present for years. The same problem would exist with hardcoded column names and has been a constant issue throughout core because different components name columns with different standards or use columns with the same name in different ways.

So is it annoying? Absolutely. Is it safer to just revert the change to save face? Yes. Is it a BC break? No.

[dneukirchen]

Although i agree that #22851 is not a bc break in technical terms, it will definitely break some extensions and forces 3dp devs to check (and change) their extensions in a patch release. Im with @laoneo here and think that it might be better to postpone the changes to 4.x.

We should care about third party devs and the issue that is fixed in #22851 is not worth the shitstorm. We can still keep/merge the fixes in #22953, #23194 (...) in 3.x, so that the core finally uses jtable column aliasing correctly.

Is there a way to keep the changes from #22851 in 3.x without getting a shitstorm from 3dp devs?
Might it help if we create an article and communicate the changes via official channels?

• what extension devs need to check/change
• what changed and why
• why this is not a bc break

[ggppdk]

Is there a way to keep the changes from #22851 in 3.x without getting a shitstorm from 3dp devs?

I think the solution exists a few lines above , look at the sanity check of checked_out column

// If the table is checked out by another user, drop it and ..
if (property_exists($table, 'checked_out') && $table->checked_out  && ($table->checked_out != ...

So in our case for the ... "published" column
This code

// Prune items that are already at the given state
if ($table->get($table->getColumnAlias('published'), $value) == $value)

will become

/**
 * Prune items that are already at the given state.  Note: Only models whose table correctly
 * sets 'published' column alias (if different than published) will benefit from this
 */
$published_col = $table->getColumnAlias('published');

if (property_exists($table, $published_col) && $table->get($published_col, $value) == $value)

[mbabker]

The checked out check needs fixing too in order to support column aliasing.

We also need to be careful how much we rely on Table::getColumnAlias() because it is NOT part of TableInterface (this should be added to the interface in 4.0).

[ggppdk]

About column aliasing of checked_out, ok
and it already done in some places

just i am a little of encouraging the use of an alias for checked_out column
as it may introduce some bug on first try, (i mean if someone actually decides to make an alias for this column)

About Table::getColumnAlias()

We also need to be careful how much we rely on Table::getColumnAlias() because it is NOT part of TableInterface (this should be added to the interface in 4.0).

		$hitsAlias = $this->table->getColumnAlias('hits');
		$checkedOutField = $table->getColumnAlias('checked_out');
		$orderingField = $this->table->getColumnAlias('ordering');

https://github.com/joomla/joomla-cms/blob/staging/libraries/src/MVC/Model/AdminModel.php#L453
https://github.com/joomla/joomla-cms/blob/staging/libraries/src/MVC/Model/AdminModel.php#L745
https://github.com/joomla/joomla-cms/blob/staging/libraries/src/MVC/Model/AdminModel.php#L1066
AdminModel already use it in other places, so we already rely on it, so surely it should be added the interface TableInterface

So is it ok to do the change i suggested ?
i have made a PR

[mbabker]

AdminModel already use it in other places, so we already rely on it, so surely it should be added the interface TableInterface

Adding methods to an interface is a B/C break, hence the reason I said it should be done in 4.0.

Until it is part of the interface, realistically you need to wrap things in method_exists() or instanceof checks (and probably log a deprecation warning somewhere saying "you need to have this in your class chain if you don't already" for best developer experience assuming someone actually looks at logs). Otherwise, you are coding against a concrete implementation of that interface and having the interface becomes pointless.

So I'm not saying don't use the feature. I am saying to do things right. We have an interface, we should be treating our code as working against that interface unless you know with 100% certainty you are working with a specific concrete implementation of that interface. You cannot make that assumption in the MVC library classes; you can relatively safely make that assumption in a component's model classes.

[Bakual]

Until it is part of the interface, realistically you need to wrap things in method_exists()

While true, we already use it in batchCopy. I'd say it's save to also use in batchMove. If we add such a wrapper and notice here, we should then do it in all 4 places. That actually is valid and should be done in another PR as it is another issue than this issue here.

I'm in favor of using the suggestion from @ggppdk instead of reverting the code. That sounds like the better plan moving forward (assuming it works). So @laoneo do you want to adapt the PR or does @ggppdk want to create a second PR?

[mbabker]

The PR @ggppdk already did (#23200) is fine. Like I said, all I wanted to do was point out the issue with coding against a concrete class when there is an interface that should be used.

[Bakual]

Ah crap, didn't see that he already did it. 👍
So we need a second PR to wrap all instances of that getColumnAlias call into method_exists or similar. Any takers? 😄

[alikon]

BC break (techincally/theoretically) or whatever ... this to to me smells like our testing system is broken