-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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 state not possible to be changed to unpublish, when Model's (J)Table does not set correct alias for 'published' column #23200
Conversation
…et correct alias for 'published' column
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 |
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). |
This PR still uses the same table api ... plus fallback to old behaviour
i agree it would be inconsistent, i would call this inconsistency a fallback behavior anyway i do not care much of this PR is merged |
I think about the "inconsistency" (fallback to old behavior) we can do this this way it will make it easier to have the relevant table classes be patched eventually |
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 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.
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 |
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:
|
Co-Authored-By: ggppdk <ggppdk@users.noreply.github.com>
Co-Authored-By: ggppdk <ggppdk@users.noreply.github.com>
All of that seems fair. |
I have tested this item ✅ successfully on be3a21e This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/23200. |
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 but to answer your question
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
So i would say just keep the fix made by PR |
I have tested this item ✅ successfully on be3a21e This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/23200. |
RTC. can go in stable ? This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/23200. |
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? |
@hrtrulz this PR will fix behavior for third party extensions not using "published" as column name. True that documentation could be improved at this point! (as column alias introduced in Joomla 3.4...) |
@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()
With link to Column Alias docs page. |
@JoomliC thanks for updating JDocs. |
First docs edit... ;-) |
@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". |
Well, if there's a public repository, of course i could help. ;-) |
Pull Request for #23194, #23193, #23192, #23191, #23197 and quoting description from PR #23197
Summary of Changes
Only If model's Table correct sets the column alias of 'published' column
only then exclude the records having already the new state from the array of records to be changed
Testing Instructions
Try to unpublish records of type
com_users Notes
com_messages
com_banners
and any other that has a state column with name different than 'published' but Model's Table does not set the 'published' column alias correctly
Expected result
records are unpublished
Actual result
records can not be unpublished
Documentation Changes Required
Developer should always set 'published' column alias at Model's Table as a good practice, if different than default