Skip to content
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

[4.2] Convert joomla action log plugin to services #37788

Merged
merged 27 commits into from May 30, 2022

Conversation

laoneo
Copy link
Member

@laoneo laoneo commented May 13, 2022

Summary of Changes

Converts the joomla action log plugin to services.

The action log component changes from #37592 are included here too, otherwise we will have a conflict after merge.

@nikosdion you may want to have a look here too.

Testing Instructions

  • Enable the joomla action log plugin
  • Save an article
  • Go to /administrator/index.php?option=com_actionlogs&view=actionlogs

Actual result BEFORE applying this Pull Request

Action log entry is written for an article create action.

Expected result AFTER applying this Pull Request

Action log entry is written for an article create action.

@richard67
Copy link
Member

@laoneo It has some CS errors: https://ci.joomla.org/joomla/joomla-cms/54000/1/6

@toivo
Copy link
Contributor

toivo commented May 14, 2022

I have tested this item ✅ successfully on 02b0b6e

Tested successfully in Joomla 4.2.0-alpha3-dev of 13 May.


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

@laoneo laoneo marked this pull request as ready for review May 15, 2022 17:14
Co-authored-by: Richard Fath <richard67@users.noreply.github.com>
@nikosdion
Copy link
Contributor

Sorry for the very long delay, I am refactoring Joomla's Two Factor Authentication.

Overall it works fine for me, I juts have one question. Since we are going into the trouble of converting the plugin, shouldn't we also convert it to use SubscriberInterface?

At some point in the future #36578 will be merged in the core. Having the plugin use SubscriberInterface would make quick work of converting it once the concrete events classes are written (phase 2 of the work on that PR). It could even serve as a proof of the concept in the core.

Totally optional, just thinking out loud.

@laoneo
Copy link
Member Author

laoneo commented May 16, 2022

I guess when #36578 got merged, then we can use the SubscriberInterface. Till then we have to use the legacy subscribing way. But SubscriberInterface is definitely the way to go.

plugins/actionlog/joomla/services/provider.php Outdated Show resolved Hide resolved
plugins/actionlog/joomla/services/provider.php Outdated Show resolved Hide resolved
@sandewt
Copy link
Contributor

sandewt commented May 30, 2022

<?xml version="1.0" encoding="UTF-8"?>

Suggestion, lower case: encoding="utf-8"

(I don't know if this is related to this issue ?)

@laoneo
Copy link
Member Author

laoneo commented May 30, 2022

To which issue?

@sandewt
Copy link
Contributor

sandewt commented May 30, 2022

To which issue?

I mean, change here too.

laoneo and others added 3 commits May 30, 2022 11:11
Co-authored-by: heelc29 <66922325+heelc29@users.noreply.github.com>
…/joomla-cms into j4/plugins/actionlog/joomla
@heelc29
Copy link
Contributor

heelc29 commented May 30, 2022

I have tested this item ✅ successfully on a878b87


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

@laoneo
Copy link
Member Author

laoneo commented May 30, 2022

@sandewt changed it. Can you give this plugin also a test?

@sandewt
Copy link
Contributor

sandewt commented May 30, 2022

Can you give this plugin also a test?

I'm going to do it, but see my comment what I came across during testing.

@alikon
Copy link
Contributor

alikon commented May 30, 2022

I have tested this item ✅ successfully on a878b87


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

@alikon
Copy link
Contributor

alikon commented May 30, 2022

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label May 30, 2022
@sandewt
Copy link
Contributor

sandewt commented May 30, 2022

I have tested this item ✅ successfully on a878b87

Joomla! 4.2.0-alpha3-dev Development


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

@roland-d roland-d merged commit 00b33e4 into joomla:4.2-dev May 30, 2022
@roland-d roland-d deleted the j4/plugins/actionlog/joomla branch May 30, 2022 19:27
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label May 30, 2022
@roland-d
Copy link
Contributor

Thanks everybody

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet