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

Migrate OC_Group post_removeFromGroup hook to actual event object #21738

Merged

Conversation

MorrisJobke
Copy link
Member

@MorrisJobke MorrisJobke commented Jul 7, 2020

Ref #14552

This adds a BeforeUserRemovedEvent to the LDAP backend because it was missing. It's not really before, but we don't have the before state.

Problem with this code:

The setup fails with this exception and I don't really know why:

logs
{
    "reqId": "eHbh95rpFH5mtg7wBVQI",
    "level": 3,
    "time": "2020-07-07T19:31:40+00:00",
    "remoteAddr": "127.0.0.1",
    "user": "--",
    "app": "index",
    "method": "POST",
    "url": "/index.php",
    "message": {
        "Exception": "Error",
        "Message": "Call to a member function getUID() on null",
        "Code": 0,
        "Trace": [
            {
                "file": "/Users/morris/Projects/nextcloud/server/core/Controller/SetupController.php",
                "line": 75,
                "function": "install",
                "class": "OC\\Setup",
                "type": "->",
                "args": [
                    "*** sensitive parameters replaced ***"
                ]
            },
            {
                "file": "/Users/morris/Projects/nextcloud/server/lib/base.php",
                "line": 955,
                "function": "run",
                "class": "OC\\Core\\Controller\\SetupController",
                "type": "->",
                "args": [
                    "*** sensitive parameters replaced ***"
                ]
            },
            {
                "file": "/Users/morris/Projects/nextcloud/server/index.php",
                "line": 37,
                "function": "handleRequest",
                "class": "OC",
                "type": "::",
                "args": []
            }
        ],
        "File": "/Users/morris/Projects/nextcloud/server/lib/private/Setup.php",
        "Line": 436,
        "CustomMessage": "--"
    },
    "userAgent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_5) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/13.1.1 Safari/605.1.15",
    "version": "20.0.0.0"
}

apps/user_ldap/lib/Jobs/UpdateGroups.php Outdated Show resolved Hide resolved
lib/private/User/User.php Outdated Show resolved Hide resolved
@juliusknorr
Copy link
Member

@MorrisJobke
Copy link
Member Author

So #22015 touched also this code. Let me rebase and resolve the code. Should make everything more clean and use then the proper events.

cc @nickvergessen

@MorrisJobke MorrisJobke force-pushed the techdebt/14552/migrate-OC_Group-post_removeFromGroup branch from 75def24 to 38cb619 Compare July 30, 2020 08:14
Ref #14552

This adds a BeforeUserRemovedEvent to the LDAP backend because it was missing. It's not really before, but we don't have the before state.

Signed-off-by: Morris Jobke <hey@morrisjobke.de>
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
… cases

Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@MorrisJobke MorrisJobke force-pushed the techdebt/14552/migrate-OC_Group-post_removeFromGroup branch from 38cb619 to 084f506 Compare July 30, 2020 08:21
@MorrisJobke MorrisJobke added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jul 30, 2020
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦈

Fine by me, although I really loved the before event. Now I see that it can't be triggered all the time, but for the things where it works, its nicer.

@blizzz
Copy link
Member

blizzz commented Aug 7, 2020

I am not sure why you deprecate BeforeUserRemovedEvent?

@MorrisJobke
Copy link
Member Author

I am not sure why you deprecate BeforeUserRemovedEvent?

Was discussed in #21738 (comment) - as there is no way to guarantee it in all cases it's better to use only the one that reliably fires. And if there is really the need for an before event we can evaluate again on this and check out what is needed there and how to accomplish this.

@blizzz
Copy link
Member

blizzz commented Aug 7, 2020

I am not sure why you deprecate BeforeUserRemovedEvent?

Was discussed in #21738 (comment) - as there is no way to guarantee it in all cases it's better to use only the one that reliably fires. And if there is really the need for an before event we can evaluate again on this and check out what is needed there and how to accomplish this.

make sense. let's then add info into the PHP doc so that any developer thinking about using it knows what it means, instead of being left puzzled.

Copy link
Member

@blizzz blizzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apart of the suggestion to provide brief info on the deprecation into doc block

Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@MorrisJobke
Copy link
Member Author

apart of the suggestion to provide brief info on the deprecation into doc block

Added.

@MorrisJobke MorrisJobke merged commit 54726d5 into master Aug 7, 2020
@MorrisJobke MorrisJobke deleted the techdebt/14552/migrate-OC_Group-post_removeFromGroup branch August 7, 2020 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants