-
Notifications
You must be signed in to change notification settings - Fork 2
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
IBX-2493: Added option to invite other users to DXP & register from invitation #35
Conversation
a47e658
to
7204541
Compare
src/lib/ConfigResolver/ConfigurableRegistrationContentTypeLoader.php
Outdated
Show resolved
Hide resolved
src/lib/ConfigResolver/ConfigurableRegistrationContentTypeLoader.php
Outdated
Show resolved
Hide resolved
src/contracts/Invitation/Exception/InvitationAlreadyExistsException.php
Outdated
Show resolved
Hide resolved
342ccdd
to
fcdcf9d
Compare
ebc8470
to
1d19c34
Compare
1d19c34
to
b50a782
Compare
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.
Integration tests would be awesome.
autoconfigure: true | ||
public: false | ||
arguments: | ||
- '@=service("kernel").locateResource("@IbexaUserBundle/Resources/config/storage/schema.yaml")' |
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.
This may allow developers to overwrite our schema. Shouldn't we hardcode it instead?
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.
If someone wants to shot himself in the foot, he can. This is how we do it all across the codebase, so maybe this is just a feature ;)
->setDefaults([ | ||
'choice_loader' => ChoiceList::lazy( | ||
$this, | ||
fn () => $this->loadFilteredRoles(), |
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.
Missing third argument ($vary
). It''s not immediately visible here, but this choice list depends on current user context.
This means that this form type, used elsewhere on page, may contain incorrect list.
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.
All changed to CallbackChoiceLoader
.
use Symfony\Component\Form\Extension\Core\Type\ChoiceType; | ||
use Symfony\Component\OptionsResolver\OptionsResolver; | ||
|
||
final class SiteAccessChoiceType extends AbstractType |
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.
Tbh it's not a User related form type. This could be extracted to core. @alongosz ?
This may or may not relate to other form types in this PR.
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.
Core does not include a single Type and I think it should stay this way. Even more, Siteaccesses are not part of the contracts there, so I would rather leave them here as there is no good place for that and SectionsChoiceType. I will just move them to Invitation
namespace to be clear about the intent.
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 don't mind adding a form type, but it seems generic, that's all. Ability to select site access is not limited to ibexa/user
package.
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.
Totally agree, there is just no good place for it atm in my opinion. (aside from adminUI maybe, but we have reversed dependency here).
Whats here:
Additional PRs:
ibexa/content-forms#29
ibexa/admin-ui#396
Checklist:
$ composer fix-cs
)