Skip to content

[5.2] Catch exception to get the user in the action log model#44358

Merged
pe7er merged 6 commits intojoomla:5.2-devfrom
Digital-Peak:actionlog/user
Dec 2, 2024
Merged

[5.2] Catch exception to get the user in the action log model#44358
pe7er merged 6 commits intojoomla:5.2-devfrom
Digital-Peak:actionlog/user

Conversation

@laoneo
Copy link
Member

@laoneo laoneo commented Oct 27, 2024

Pull Request for Issue #42373.

Summary of Changes

Catches an exception when the user factory is not set in the action log model. This is basically against the principle that we want to have the dependencies injected, but it is good for the transition period till all extensions are up to the new architecture.

Testing Instructions

Happens only on 3rd party extensions, see issue.

Actual result BEFORE applying this Pull Request

Extension produces an error when writing to the action log.

Expected result AFTER applying this Pull Request

Extension works and action log is written to the database.

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

@richard67 richard67 added the bug label Oct 27, 2024
@ssnobben
Copy link

Yes the solution for seems working ok now after this fix the error message gone. Great! This still an issue wihth the Finder plg that have to be disable to though.. Plugin\Finder\Content\Extension\Content::onFinderAfterSave() error #42617 #42617

…el.php

Co-authored-by: Quy <quy@nomonkeybiz.com>
@laoneo
Copy link
Member Author

laoneo commented Oct 28, 2024

The finder issue is not addressed by this pr.

@ssnobben
Copy link

ssnobben commented Oct 28, 2024

The finder issue is not addressed by this pr.

Yes ok. Thks Allon . Can you reopen this one for Finder?

@laoneo
Copy link
Member Author

laoneo commented Oct 28, 2024

@ssnobben done

@ssnobben
Copy link

Can anyone test this one "by the boook" Joomla way so its into next release? or by code review bcs Its working now after this fix. Thks.


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

@HLeithner HLeithner changed the title Catch exception to get the user in the action log model [5.2] Catch exception to get the user in the action log model Nov 15, 2024
@Hackwar
Copy link
Member

Hackwar commented Nov 21, 2024

I have tested this item ✅ successfully on e4a6c79


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

@Hackwar
Copy link
Member

Hackwar commented Nov 21, 2024

@ssnobben if you were to mark this as successfully tested, then we could merge this into 5.2.3.

@viocassel
Copy link
Contributor

I have tested this item ✅ successfully on e4a6c79


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

@Quy
Copy link
Contributor

Quy commented Nov 23, 2024

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Nov 23, 2024
@ssnobben
Copy link

@ssnobben if you were to mark this as successfully tested, then we could merge this into 5.2.3.

All checks have passed
4 successful checks
@joomla-att01
@joomla-cms-bot
Merging is blocked @Hackwar
The base branch does not allow updates. Learn more about protected branches.

@Hackwar
Copy link
Member

Hackwar commented Nov 25, 2024

Every PR for the CMS needs 2 successful manual tests. At the time of my comment, I was test 1 and you could have been test 2. Now @viocassel has thankfully tested it. Everything else is not of concern right now.

@ssnobben
Copy link

Thanks @viocassel !

@ssnobben
Copy link

Ok so why was this important issue not merged?? I want to hear an explanation.. I have beew working with Joomla and 3pds that is important for Joomla eccosystem since 2005 and this have been the most waited fix ever...

@pe7er
Copy link
Contributor

pe7er commented Nov 26, 2024

Ok so why was this important issue not merged?? I want to hear an explanation.. I have beew working with Joomla and 3pds that is important for Joomla eccosystem since 2005 and this have been the most waited fix ever...

The second human test for this PR came in too late to include it in Joomla 5.2.2,
which was already scheduled and released earlier today, just 2.5 hours ago.

@Hackwar
Copy link
Member

Hackwar commented Nov 27, 2024

@ssnobben Your comment is completely out of line.

@ssnobben
Copy link

@ssnobben Your comment is completely out of line.

Could be but this have been an issue since 2023 until it finally got an solution. - Thu Oct 26, 2023 10:22 am reported here in Joomla world first. I work with 3pds and people have waiting for a solution for this to release their site forever...

@pe7er
Copy link
Contributor

pe7er commented Nov 27, 2024

Could be but this have been an issue since 2023 until it finally got an solution. - Thu Oct 26, 2023 10:22 am reported here in Joomla world first. I work with 3pds and people have waiting for a solution for this to release their site forever...

I understand your concern. However, every change can affect other parts of Joomla or extensions from third parties. Therefore we need to be careful with adding any code to Joomla. Every PR needs to pass one successful automated test and get two successful human tests before it can be added to the core. One week before a release, a Release Candidate is created for testing that release. At this stage, we do not add new PRs, but only fix issues with the existing code in that package. The second successful test for this PR was reported during this phase so it was too late to add it to 5.2.2.

@Hackwar
Copy link
Member

Hackwar commented Nov 27, 2024

We have issues reported as old as 2015 and serious issues for example with MFA from 2020. As unfortunate as it is that it took a year to get it ready, it is not uncommon. And the reason is very simple: We are volunteers and do this in our spare time. You were very welcome to hand in a patch/PR yourself to fix this in all that time. You are not required to first open an issue. If you find a problem, you can directly create a PR and if you then motivate a few other people to test the PR, it can actually merged pretty quickly. If you don't feel that you have the knowledge to do that, you can also hire a developer to do this for you. We don't really discriminate based on this. But if you are unwilling to either work on this yourself or to invest into this and instead want free work from the community, you have to accept that it might sometimes take years until an issue is solved.

@pe7er pe7er merged commit 09ca50a into joomla:5.2-dev Dec 2, 2024
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Dec 2, 2024
@pe7er
Copy link
Contributor

pe7er commented Dec 2, 2024

Thanks @laoneo !

@pe7er pe7er added this to the Joomla! 5.2.3 milestone Dec 2, 2024
@laoneo laoneo deleted the actionlog/user branch December 2, 2024 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants