-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
event on webfinger request #21817
event on webfinger request #21817
Conversation
That is not me ;) |
@daita I'm not fully aware of the webfinger spec. But maybe your API is a bit overkill? As far as I undersand a webfinger request comes in giving a resource and an optional relation. And we spit back out json right? I take it this is to make social handling a bit nicer? |
Yes, exactly. It would also make multiple apps answering to the same request. Of course, not about the same protocol |
ok, I got your point now :) |
5efc448
to
c2aedfb
Compare
So, fresh new commit, a lot simpler. Only one Model
no more method to fill some missing data as it should be useless now. Apps just feed links with an array or a JsonSerializable class. |
@daita Mind to fix the PHP CS warnings? |
There are still some code style warning. See https://github.com/nextcloud/server/pull/21817/checks?check_run_id=923955210 for the diff or just run |
1685e28
to
82286e8
Compare
yes, saw that yesterday night, but went to sleep before fixing it. should be better now. |
99e9b7f
to
eb204b5
Compare
It's a private event. What's the purpose of this? |
eb204b5
to
ae63384
Compare
rebased, and moving the event to public. |
c8f60b5
to
f47b83b
Compare
lib/private/WellKnown/Exceptions/NotManagedWellKnownRequestException.php
Outdated
Show resolved
Hide resolved
lib/private/WellKnown/Exceptions/NotManagedWellKnownRequestException.php
Outdated
Show resolved
Hide resolved
* @package OCP\WellKnown\Events | ||
* @since 21.0.0 | ||
*/ | ||
class WellKnownEvent extends Event { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WellKnownRequestEvent
still doesn't match the naming scheme. Your event doesn't have a verb. Well-known request ??? received?
Signed-off-by: Maxence Lange <maxence@artificial-owl.com>
c57678b
to
382d2f9
Compare
Signed-off-by: Maxence Lange <maxence@artificial-owl.com>
@ChristophWurst event renamed to WellKnownRequestEvent |
🤖 beep boop beep 🤖 Here are the logs for the failed build: Status of 345: failureintegration-federation_features
Show full log
integration-filesdrop-features
Show full log
|
Signed-off-by: Maxence Lange <maxence@artificial-owl.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking aloud a bit.
Maybe this isn't the ideal API design. You're using an event to trigger an action. This is imperative thinking. Like apps do your thing and update the object that I'm giving you.
But what you essentially want to do is offer the apps a way to hook into the process of a well known request and add their custom things to the response.
How about we move this from events to registered handlers so that apps can register their webfinger request handlers in the bootstrap registration context. Those handlers are of a simple interface type that passes in the request and gets back the data we allow apps to specify.
Then, when a wellknown request is received, we ask the registration context for all of those handlers, iterate over them and ask them for their data, which we then combine and send back.
Does that make sense?
In general terms I'm really not a fan of using events where the emitting code expects some kind of mutation, either on the event or any other (global) state. Events should be more passive. Like just hey, this happened. do whatever you want with this information but don't expect any other code to even listen to this nor make any modifications.
Note to myself
|
The idea is to generate an event so that apps can add their own parts to the response on a webfinger requestNow, event is generated when request is made on .well-known/webfinger and .well-known/nodeinfo