add anonymous users support

[htaoufikallah]

No description provided.

[htaoufikallah]

Hello @bpolaszek, do you have any suggestion to fix those failing checks with --prefer-stable option?

[bpolaszek]

Hello @htaoufikallah - thank you for this PR!

I'm currently pretty busy and I can't review this at the very moment, but that's definitely something I wanted to work on.

I'll try to check that out within the next days/weeks, please be patient.

[htaoufikallah]

No worries @bpolaszek I'll wait, thank you

[bpolaszek]

Hello @htaoufikallah, thanks for your patience and for the time you took to improve this library! 🙂

Allowing anonymous users is something that I wanted to do for a while, in order to send notifications to people that did not necessarily sign up to the app. They could be targeted through different, arbitrary attributes than the associated user id (main controller expecting an optional, additional payload of data associated to the subscription).

The main concern that prevented me from doing it, is that it would break existing implementations, because interfaces have to be changed to allow nullable UserInterface (and so do the implementations). There's a disclaimer in the README saying that this lib should not to be considered as stable and changes have to be expected, but a README doesn't prevent a bad composer update and I'd like to avoid breaking things on others' apps as much as possible.

What I notice from your implementation are:

• It actually breaks existing implementations, because of the contract change
• It introduces a new ambiguity that the implementor should resolve, which is "should I replace an existing anonymous subscription once the user logs in?" - and this ambiguity has to be documented.

Those issues are legit, however I think we can address them in a different way.
The approach I was just thinking about is to:

• Leave the existing contracts unchanged: UserInterface remains mandatory.
• Introduce in the library an AnonymousUser class, marked @internal, implementing Symfony\Component\Security\Core\User\UserInterface (without any Doctrine annotations, because persistence is the implementor's responsibility).
• 1st lines of BenTools\WebPushBundle\Action\RegisterSubscriptionAction::__invoke become this:

if (!in_array($request->getMethod(), ['POST', 'DELETE'])) {
thrownewMethodNotAllowedHttpException(['POST', 'DELETE']);
}
$data = json_decode($request->getContent(), true);
$subscription = $data['subscription'] ?? [];
$options = $data['options'] ?? [];
$user = $user ?? newAnonymousUser($options); // options being an arbitrary array of data sent by the client

This way, the main controller always provide a valid UserInterface implementation.

• Provide documentation on how to implement and/or register a UserSubscriptionManagerInterface for AnonymousUser (implementor can match this class against instanceof operators, and deal with the arbitrary data passed by the user agent. This means implementor can either register 2 managers, or 1 for both classes by discriminating user classes under the hood).

Regarding anonymous -> logged-in upgrade:

• Collisions can already be managed by the implementor with the way they implement UserSubscriptionManagerInterface::hash and UserSubscriptionManagerInterface::factory: you can hash the subscription payload without the user information, and check for an existing anonymous entry when implementing the factory() method.

Unless there's something I didn't spot out, this would help bringing this feature without breaking existing apps.

What do you think?

[htaoufikallah]

Hello @bpolaszek,
Thanks for taking the time to review this PR I understand your concern very well, which I missed while preparing this PR.
I read your approach many times and I think I understand it very well, so I'll give it a try and I'll update this PR with the required changes 🙂

[bobemoe]

This looks like just what I need. What is stopping it being merged? Happy to help here, I am setting up a new project where I would need this so could test this at the same time?

[bpolaszek]

Sorry for the delay!
Would you mind implementing default methods of AnonymousUser, i.e. getRoles should return an empty array, etc.

Thanks!
Ben

[htaoufikallah]

please recheck and let me know if anything need to be done.

[bpolaszek]

Approved too fast: missing this in AnonymousUser:

publicfunctiongetUserIdentifier(): string
{
return'';
}

This method has been introduced in SF6's UserInterface.

[htaoufikallah]

No description provided.

[htaoufikallah]

Hello @bpolaszek, do you have any suggestion to fix those failing checks with --prefer-stable option?

[bpolaszek]

Hello @htaoufikallah - thank you for this PR!

I'm currently pretty busy and I can't review this at the very moment, but that's definitely something I wanted to work on.

I'll try to check that out within the next days/weeks, please be patient.

[htaoufikallah]

No worries @bpolaszek I'll wait, thank you

[bpolaszek]

Hello @htaoufikallah, thanks for your patience and for the time you took to improve this library! 🙂

Allowing anonymous users is something that I wanted to do for a while, in order to send notifications to people that did not necessarily sign up to the app. They could be targeted through different, arbitrary attributes than the associated user id (main controller expecting an optional, additional payload of data associated to the subscription).

The main concern that prevented me from doing it, is that it would break existing implementations, because interfaces have to be changed to allow nullable UserInterface (and so do the implementations). There's a disclaimer in the README saying that this lib should not to be considered as stable and changes have to be expected, but a README doesn't prevent a bad composer update and I'd like to avoid breaking things on others' apps as much as possible.

What I notice from your implementation are:

• It actually breaks existing implementations, because of the contract change
• It introduces a new ambiguity that the implementor should resolve, which is "should I replace an existing anonymous subscription once the user logs in?" - and this ambiguity has to be documented.

Those issues are legit, however I think we can address them in a different way.
The approach I was just thinking about is to:

• Leave the existing contracts unchanged: UserInterface remains mandatory.
• Introduce in the library an AnonymousUser class, marked @internal, implementing Symfony\Component\Security\Core\User\UserInterface (without any Doctrine annotations, because persistence is the implementor's responsibility).
• 1st lines of BenTools\WebPushBundle\Action\RegisterSubscriptionAction::__invoke become this:

if (!in_array($request->getMethod(), ['POST', 'DELETE'])) {
thrownewMethodNotAllowedHttpException(['POST', 'DELETE']);
}
$data = json_decode($request->getContent(), true);
$subscription = $data['subscription'] ?? [];
$options = $data['options'] ?? [];
$user = $user ?? newAnonymousUser($options); // options being an arbitrary array of data sent by the client

This way, the main controller always provide a valid UserInterface implementation.

• Provide documentation on how to implement and/or register a UserSubscriptionManagerInterface for AnonymousUser (implementor can match this class against instanceof operators, and deal with the arbitrary data passed by the user agent. This means implementor can either register 2 managers, or 1 for both classes by discriminating user classes under the hood).

Regarding anonymous -> logged-in upgrade:

• Collisions can already be managed by the implementor with the way they implement UserSubscriptionManagerInterface::hash and UserSubscriptionManagerInterface::factory: you can hash the subscription payload without the user information, and check for an existing anonymous entry when implementing the factory() method.

Unless there's something I didn't spot out, this would help bringing this feature without breaking existing apps.

What do you think?

[htaoufikallah]

Hello @bpolaszek,
Thanks for taking the time to review this PR I understand your concern very well, which I missed while preparing this PR.
I read your approach many times and I think I understand it very well, so I'll give it a try and I'll update this PR with the required changes 🙂

[bobemoe]

This looks like just what I need. What is stopping it being merged? Happy to help here, I am setting up a new project where I would need this so could test this at the same time?

[bpolaszek]

Sorry for the delay!
Would you mind implementing default methods of AnonymousUser, i.e. getRoles should return an empty array, etc.

Thanks!
Ben

[htaoufikallah]

please recheck and let me know if anything need to be done.

[bpolaszek]

Approved too fast: missing this in AnonymousUser:

publicfunctiongetUserIdentifier(): string
{
return'';
}

This method has been introduced in SF6's UserInterface.

[htaoufikallah]

No description provided.

[htaoufikallah]

Hello @bpolaszek, do you have any suggestion to fix those failing checks with --prefer-stable option?

[bpolaszek]

Hello @htaoufikallah - thank you for this PR!

I'm currently pretty busy and I can't review this at the very moment, but that's definitely something I wanted to work on.

I'll try to check that out within the next days/weeks, please be patient.

[htaoufikallah]

No worries @bpolaszek I'll wait, thank you

[bpolaszek]

Hello @htaoufikallah, thanks for your patience and for the time you took to improve this library! 🙂

Allowing anonymous users is something that I wanted to do for a while, in order to send notifications to people that did not necessarily sign up to the app. They could be targeted through different, arbitrary attributes than the associated user id (main controller expecting an optional, additional payload of data associated to the subscription).

The main concern that prevented me from doing it, is that it would break existing implementations, because interfaces have to be changed to allow nullable UserInterface (and so do the implementations). There's a disclaimer in the README saying that this lib should not to be considered as stable and changes have to be expected, but a README doesn't prevent a bad composer update and I'd like to avoid breaking things on others' apps as much as possible.

What I notice from your implementation are:

• It actually breaks existing implementations, because of the contract change
• It introduces a new ambiguity that the implementor should resolve, which is "should I replace an existing anonymous subscription once the user logs in?" - and this ambiguity has to be documented.

Those issues are legit, however I think we can address them in a different way.
The approach I was just thinking about is to:

• Leave the existing contracts unchanged: UserInterface remains mandatory.
• Introduce in the library an AnonymousUser class, marked @internal, implementing Symfony\Component\Security\Core\User\UserInterface (without any Doctrine annotations, because persistence is the implementor's responsibility).
• 1st lines of BenTools\WebPushBundle\Action\RegisterSubscriptionAction::__invoke become this:

if (!in_array($request->getMethod(), ['POST', 'DELETE'])) {
thrownewMethodNotAllowedHttpException(['POST', 'DELETE']);
}
$data = json_decode($request->getContent(), true);
$subscription = $data['subscription'] ?? [];
$options = $data['options'] ?? [];
$user = $user ?? newAnonymousUser($options); // options being an arbitrary array of data sent by the client

This way, the main controller always provide a valid UserInterface implementation.

• Provide documentation on how to implement and/or register a UserSubscriptionManagerInterface for AnonymousUser (implementor can match this class against instanceof operators, and deal with the arbitrary data passed by the user agent. This means implementor can either register 2 managers, or 1 for both classes by discriminating user classes under the hood).

Regarding anonymous -> logged-in upgrade:

• Collisions can already be managed by the implementor with the way they implement UserSubscriptionManagerInterface::hash and UserSubscriptionManagerInterface::factory: you can hash the subscription payload without the user information, and check for an existing anonymous entry when implementing the factory() method.

Unless there's something I didn't spot out, this would help bringing this feature without breaking existing apps.

What do you think?

[htaoufikallah]

Hello @bpolaszek,
Thanks for taking the time to review this PR I understand your concern very well, which I missed while preparing this PR.
I read your approach many times and I think I understand it very well, so I'll give it a try and I'll update this PR with the required changes 🙂

[bobemoe]

This looks like just what I need. What is stopping it being merged? Happy to help here, I am setting up a new project where I would need this so could test this at the same time?

[bpolaszek]

Sorry for the delay!
Would you mind implementing default methods of AnonymousUser, i.e. getRoles should return an empty array, etc.

Thanks!
Ben

[htaoufikallah]

please recheck and let me know if anything need to be done.

[bpolaszek]

Approved too fast: missing this in AnonymousUser:

publicfunctiongetUserIdentifier(): string
{
return'';
}

This method has been introduced in SF6's UserInterface.

[htaoufikallah]

No description provided.

[htaoufikallah]

Hello @bpolaszek, do you have any suggestion to fix those failing checks with --prefer-stable option?

[bpolaszek]

Hello @htaoufikallah - thank you for this PR!

I'm currently pretty busy and I can't review this at the very moment, but that's definitely something I wanted to work on.

I'll try to check that out within the next days/weeks, please be patient.

[htaoufikallah]

No worries @bpolaszek I'll wait, thank you

[bpolaszek]

Hello @htaoufikallah, thanks for your patience and for the time you took to improve this library! 🙂

Allowing anonymous users is something that I wanted to do for a while, in order to send notifications to people that did not necessarily sign up to the app. They could be targeted through different, arbitrary attributes than the associated user id (main controller expecting an optional, additional payload of data associated to the subscription).

The main concern that prevented me from doing it, is that it would break existing implementations, because interfaces have to be changed to allow nullable UserInterface (and so do the implementations). There's a disclaimer in the README saying that this lib should not to be considered as stable and changes have to be expected, but a README doesn't prevent a bad composer update and I'd like to avoid breaking things on others' apps as much as possible.

What I notice from your implementation are:

• It actually breaks existing implementations, because of the contract change
• It introduces a new ambiguity that the implementor should resolve, which is "should I replace an existing anonymous subscription once the user logs in?" - and this ambiguity has to be documented.

Those issues are legit, however I think we can address them in a different way.
The approach I was just thinking about is to:

• Leave the existing contracts unchanged: UserInterface remains mandatory.
• Introduce in the library an AnonymousUser class, marked @internal, implementing Symfony\Component\Security\Core\User\UserInterface (without any Doctrine annotations, because persistence is the implementor's responsibility).
• 1st lines of BenTools\WebPushBundle\Action\RegisterSubscriptionAction::__invoke become this:

if (!in_array($request->getMethod(), ['POST', 'DELETE'])) {
thrownewMethodNotAllowedHttpException(['POST', 'DELETE']);
}
$data = json_decode($request->getContent(), true);
$subscription = $data['subscription'] ?? [];
$options = $data['options'] ?? [];
$user = $user ?? newAnonymousUser($options); // options being an arbitrary array of data sent by the client

This way, the main controller always provide a valid UserInterface implementation.

• Provide documentation on how to implement and/or register a UserSubscriptionManagerInterface for AnonymousUser (implementor can match this class against instanceof operators, and deal with the arbitrary data passed by the user agent. This means implementor can either register 2 managers, or 1 for both classes by discriminating user classes under the hood).

Regarding anonymous -> logged-in upgrade:

• Collisions can already be managed by the implementor with the way they implement UserSubscriptionManagerInterface::hash and UserSubscriptionManagerInterface::factory: you can hash the subscription payload without the user information, and check for an existing anonymous entry when implementing the factory() method.

Unless there's something I didn't spot out, this would help bringing this feature without breaking existing apps.

What do you think?

[htaoufikallah]

Hello @bpolaszek,
Thanks for taking the time to review this PR I understand your concern very well, which I missed while preparing this PR.
I read your approach many times and I think I understand it very well, so I'll give it a try and I'll update this PR with the required changes 🙂

[bobemoe]

This looks like just what I need. What is stopping it being merged? Happy to help here, I am setting up a new project where I would need this so could test this at the same time?

[bpolaszek]

Sorry for the delay!
Would you mind implementing default methods of AnonymousUser, i.e. getRoles should return an empty array, etc.

Thanks!
Ben

[htaoufikallah]

please recheck and let me know if anything need to be done.

[bpolaszek]

Approved too fast: missing this in AnonymousUser:

publicfunctiongetUserIdentifier(): string
{
return'';
}

This method has been introduced in SF6's UserInterface.

[htaoufikallah]

No description provided.

[htaoufikallah]

Hello @bpolaszek, do you have any suggestion to fix those failing checks with --prefer-stable option?

[bpolaszek]

Hello @htaoufikallah - thank you for this PR!

I'm currently pretty busy and I can't review this at the very moment, but that's definitely something I wanted to work on.

I'll try to check that out within the next days/weeks, please be patient.

[htaoufikallah]

No worries @bpolaszek I'll wait, thank you

[bpolaszek]

Hello @htaoufikallah, thanks for your patience and for the time you took to improve this library! 🙂

Allowing anonymous users is something that I wanted to do for a while, in order to send notifications to people that did not necessarily sign up to the app. They could be targeted through different, arbitrary attributes than the associated user id (main controller expecting an optional, additional payload of data associated to the subscription).

The main concern that prevented me from doing it, is that it would break existing implementations, because interfaces have to be changed to allow nullable UserInterface (and so do the implementations). There's a disclaimer in the README saying that this lib should not to be considered as stable and changes have to be expected, but a README doesn't prevent a bad composer update and I'd like to avoid breaking things on others' apps as much as possible.

What I notice from your implementation are:

• It actually breaks existing implementations, because of the contract change
• It introduces a new ambiguity that the implementor should resolve, which is "should I replace an existing anonymous subscription once the user logs in?" - and this ambiguity has to be documented.

Those issues are legit, however I think we can address them in a different way.
The approach I was just thinking about is to:

• Leave the existing contracts unchanged: UserInterface remains mandatory.
• Introduce in the library an AnonymousUser class, marked @internal, implementing Symfony\Component\Security\Core\User\UserInterface (without any Doctrine annotations, because persistence is the implementor's responsibility).
• 1st lines of BenTools\WebPushBundle\Action\RegisterSubscriptionAction::__invoke become this:

if (!in_array($request->getMethod(), ['POST', 'DELETE'])) {
thrownewMethodNotAllowedHttpException(['POST', 'DELETE']);
}
$data = json_decode($request->getContent(), true);
$subscription = $data['subscription'] ?? [];
$options = $data['options'] ?? [];
$user = $user ?? newAnonymousUser($options); // options being an arbitrary array of data sent by the client

This way, the main controller always provide a valid UserInterface implementation.

• Provide documentation on how to implement and/or register a UserSubscriptionManagerInterface for AnonymousUser (implementor can match this class against instanceof operators, and deal with the arbitrary data passed by the user agent. This means implementor can either register 2 managers, or 1 for both classes by discriminating user classes under the hood).

Regarding anonymous -> logged-in upgrade:

• Collisions can already be managed by the implementor with the way they implement UserSubscriptionManagerInterface::hash and UserSubscriptionManagerInterface::factory: you can hash the subscription payload without the user information, and check for an existing anonymous entry when implementing the factory() method.

Unless there's something I didn't spot out, this would help bringing this feature without breaking existing apps.

What do you think?

[htaoufikallah]

Hello @bpolaszek,
Thanks for taking the time to review this PR I understand your concern very well, which I missed while preparing this PR.
I read your approach many times and I think I understand it very well, so I'll give it a try and I'll update this PR with the required changes 🙂

[bobemoe]

This looks like just what I need. What is stopping it being merged? Happy to help here, I am setting up a new project where I would need this so could test this at the same time?

[bpolaszek]

Sorry for the delay!
Would you mind implementing default methods of AnonymousUser, i.e. getRoles should return an empty array, etc.

Thanks!
Ben

[htaoufikallah]

please recheck and let me know if anything need to be done.

[bpolaszek]

Approved too fast: missing this in AnonymousUser:

publicfunctiongetUserIdentifier(): string
{
return'';
}

This method has been introduced in SF6's UserInterface.

[htaoufikallah]

No description provided.

[htaoufikallah]

Hello @bpolaszek, do you have any suggestion to fix those failing checks with --prefer-stable option?

[bpolaszek]

Hello @htaoufikallah - thank you for this PR!

I'm currently pretty busy and I can't review this at the very moment, but that's definitely something I wanted to work on.

I'll try to check that out within the next days/weeks, please be patient.

[htaoufikallah]

No worries @bpolaszek I'll wait, thank you

[bpolaszek]

Hello @htaoufikallah, thanks for your patience and for the time you took to improve this library! 🙂

Allowing anonymous users is something that I wanted to do for a while, in order to send notifications to people that did not necessarily sign up to the app. They could be targeted through different, arbitrary attributes than the associated user id (main controller expecting an optional, additional payload of data associated to the subscription).

The main concern that prevented me from doing it, is that it would break existing implementations, because interfaces have to be changed to allow nullable UserInterface (and so do the implementations). There's a disclaimer in the README saying that this lib should not to be considered as stable and changes have to be expected, but a README doesn't prevent a bad composer update and I'd like to avoid breaking things on others' apps as much as possible.

What I notice from your implementation are:

• It actually breaks existing implementations, because of the contract change
• It introduces a new ambiguity that the implementor should resolve, which is "should I replace an existing anonymous subscription once the user logs in?" - and this ambiguity has to be documented.

Those issues are legit, however I think we can address them in a different way.
The approach I was just thinking about is to:

• Leave the existing contracts unchanged: UserInterface remains mandatory.
• Introduce in the library an AnonymousUser class, marked @internal, implementing Symfony\Component\Security\Core\User\UserInterface (without any Doctrine annotations, because persistence is the implementor's responsibility).
• 1st lines of BenTools\WebPushBundle\Action\RegisterSubscriptionAction::__invoke become this:

if (!in_array($request->getMethod(), ['POST', 'DELETE'])) {
thrownewMethodNotAllowedHttpException(['POST', 'DELETE']);
}
$data = json_decode($request->getContent(), true);
$subscription = $data['subscription'] ?? [];
$options = $data['options'] ?? [];
$user = $user ?? newAnonymousUser($options); // options being an arbitrary array of data sent by the client

This way, the main controller always provide a valid UserInterface implementation.

• Provide documentation on how to implement and/or register a UserSubscriptionManagerInterface for AnonymousUser (implementor can match this class against instanceof operators, and deal with the arbitrary data passed by the user agent. This means implementor can either register 2 managers, or 1 for both classes by discriminating user classes under the hood).

Regarding anonymous -> logged-in upgrade:

• Collisions can already be managed by the implementor with the way they implement UserSubscriptionManagerInterface::hash and UserSubscriptionManagerInterface::factory: you can hash the subscription payload without the user information, and check for an existing anonymous entry when implementing the factory() method.

Unless there's something I didn't spot out, this would help bringing this feature without breaking existing apps.

What do you think?

[htaoufikallah]

Hello @bpolaszek,
Thanks for taking the time to review this PR I understand your concern very well, which I missed while preparing this PR.
I read your approach many times and I think I understand it very well, so I'll give it a try and I'll update this PR with the required changes 🙂

[bobemoe]

This looks like just what I need. What is stopping it being merged? Happy to help here, I am setting up a new project where I would need this so could test this at the same time?

[bpolaszek]

Sorry for the delay!
Would you mind implementing default methods of AnonymousUser, i.e. getRoles should return an empty array, etc.

Thanks!
Ben

[htaoufikallah]

please recheck and let me know if anything need to be done.

[bpolaszek]

Approved too fast: missing this in AnonymousUser:

publicfunctiongetUserIdentifier(): string
{
return'';
}

This method has been introduced in SF6's UserInterface.

[htaoufikallah]

No description provided.

[htaoufikallah]

Hello @bpolaszek, do you have any suggestion to fix those failing checks with --prefer-stable option?

[bpolaszek]

Hello @htaoufikallah - thank you for this PR!

I'm currently pretty busy and I can't review this at the very moment, but that's definitely something I wanted to work on.

I'll try to check that out within the next days/weeks, please be patient.

[htaoufikallah]

No worries @bpolaszek I'll wait, thank you

[bpolaszek]

Hello @htaoufikallah, thanks for your patience and for the time you took to improve this library! 🙂

Allowing anonymous users is something that I wanted to do for a while, in order to send notifications to people that did not necessarily sign up to the app. They could be targeted through different, arbitrary attributes than the associated user id (main controller expecting an optional, additional payload of data associated to the subscription).

The main concern that prevented me from doing it, is that it would break existing implementations, because interfaces have to be changed to allow nullable UserInterface (and so do the implementations). There's a disclaimer in the README saying that this lib should not to be considered as stable and changes have to be expected, but a README doesn't prevent a bad composer update and I'd like to avoid breaking things on others' apps as much as possible.

What I notice from your implementation are:

• It actually breaks existing implementations, because of the contract change
• It introduces a new ambiguity that the implementor should resolve, which is "should I replace an existing anonymous subscription once the user logs in?" - and this ambiguity has to be documented.

Those issues are legit, however I think we can address them in a different way.
The approach I was just thinking about is to:

• Leave the existing contracts unchanged: UserInterface remains mandatory.
• Introduce in the library an AnonymousUser class, marked @internal, implementing Symfony\Component\Security\Core\User\UserInterface (without any Doctrine annotations, because persistence is the implementor's responsibility).
• 1st lines of BenTools\WebPushBundle\Action\RegisterSubscriptionAction::__invoke become this:

if (!in_array($request->getMethod(), ['POST', 'DELETE'])) {
thrownewMethodNotAllowedHttpException(['POST', 'DELETE']);
}
$data = json_decode($request->getContent(), true);
$subscription = $data['subscription'] ?? [];
$options = $data['options'] ?? [];
$user = $user ?? newAnonymousUser($options); // options being an arbitrary array of data sent by the client

This way, the main controller always provide a valid UserInterface implementation.

• Provide documentation on how to implement and/or register a UserSubscriptionManagerInterface for AnonymousUser (implementor can match this class against instanceof operators, and deal with the arbitrary data passed by the user agent. This means implementor can either register 2 managers, or 1 for both classes by discriminating user classes under the hood).

Regarding anonymous -> logged-in upgrade:

• Collisions can already be managed by the implementor with the way they implement UserSubscriptionManagerInterface::hash and UserSubscriptionManagerInterface::factory: you can hash the subscription payload without the user information, and check for an existing anonymous entry when implementing the factory() method.

Unless there's something I didn't spot out, this would help bringing this feature without breaking existing apps.

What do you think?

[htaoufikallah]

Hello @bpolaszek,
Thanks for taking the time to review this PR I understand your concern very well, which I missed while preparing this PR.
I read your approach many times and I think I understand it very well, so I'll give it a try and I'll update this PR with the required changes 🙂

[bobemoe]

This looks like just what I need. What is stopping it being merged? Happy to help here, I am setting up a new project where I would need this so could test this at the same time?

[bpolaszek]

Sorry for the delay!
Would you mind implementing default methods of AnonymousUser, i.e. getRoles should return an empty array, etc.

Thanks!
Ben

[htaoufikallah]

please recheck and let me know if anything need to be done.

[bpolaszek]

Approved too fast: missing this in AnonymousUser:

publicfunctiongetUserIdentifier(): string
{
return'';
}

This method has been introduced in SF6's UserInterface.

[htaoufikallah]

No description provided.

[htaoufikallah]

Hello @bpolaszek, do you have any suggestion to fix those failing checks with --prefer-stable option?

[bpolaszek]

Hello @htaoufikallah - thank you for this PR!

I'm currently pretty busy and I can't review this at the very moment, but that's definitely something I wanted to work on.

I'll try to check that out within the next days/weeks, please be patient.

[htaoufikallah]

No worries @bpolaszek I'll wait, thank you

[bpolaszek]

Hello @htaoufikallah, thanks for your patience and for the time you took to improve this library! 🙂

Allowing anonymous users is something that I wanted to do for a while, in order to send notifications to people that did not necessarily sign up to the app. They could be targeted through different, arbitrary attributes than the associated user id (main controller expecting an optional, additional payload of data associated to the subscription).

The main concern that prevented me from doing it, is that it would break existing implementations, because interfaces have to be changed to allow nullable UserInterface (and so do the implementations). There's a disclaimer in the README saying that this lib should not to be considered as stable and changes have to be expected, but a README doesn't prevent a bad composer update and I'd like to avoid breaking things on others' apps as much as possible.

What I notice from your implementation are:

• It actually breaks existing implementations, because of the contract change
• It introduces a new ambiguity that the implementor should resolve, which is "should I replace an existing anonymous subscription once the user logs in?" - and this ambiguity has to be documented.

Those issues are legit, however I think we can address them in a different way.
The approach I was just thinking about is to:

• Leave the existing contracts unchanged: UserInterface remains mandatory.
• Introduce in the library an AnonymousUser class, marked @internal, implementing Symfony\Component\Security\Core\User\UserInterface (without any Doctrine annotations, because persistence is the implementor's responsibility).
• 1st lines of BenTools\WebPushBundle\Action\RegisterSubscriptionAction::__invoke become this:

if (!in_array($request->getMethod(), ['POST', 'DELETE'])) {
thrownewMethodNotAllowedHttpException(['POST', 'DELETE']);
}
$data = json_decode($request->getContent(), true);
$subscription = $data['subscription'] ?? [];
$options = $data['options'] ?? [];
$user = $user ?? newAnonymousUser($options); // options being an arbitrary array of data sent by the client

This way, the main controller always provide a valid UserInterface implementation.

• Provide documentation on how to implement and/or register a UserSubscriptionManagerInterface for AnonymousUser (implementor can match this class against instanceof operators, and deal with the arbitrary data passed by the user agent. This means implementor can either register 2 managers, or 1 for both classes by discriminating user classes under the hood).

Regarding anonymous -> logged-in upgrade:

• Collisions can already be managed by the implementor with the way they implement UserSubscriptionManagerInterface::hash and UserSubscriptionManagerInterface::factory: you can hash the subscription payload without the user information, and check for an existing anonymous entry when implementing the factory() method.

Unless there's something I didn't spot out, this would help bringing this feature without breaking existing apps.

What do you think?

[htaoufikallah]

Hello @bpolaszek,
Thanks for taking the time to review this PR I understand your concern very well, which I missed while preparing this PR.
I read your approach many times and I think I understand it very well, so I'll give it a try and I'll update this PR with the required changes 🙂

[bobemoe]

This looks like just what I need. What is stopping it being merged? Happy to help here, I am setting up a new project where I would need this so could test this at the same time?

[bpolaszek]

Sorry for the delay!
Would you mind implementing default methods of AnonymousUser, i.e. getRoles should return an empty array, etc.

Thanks!
Ben

[htaoufikallah]

please recheck and let me know if anything need to be done.

[bpolaszek]

Approved too fast: missing this in AnonymousUser:

publicfunctiongetUserIdentifier(): string
{
return'';
}

This method has been introduced in SF6's UserInterface.