zikula / core

Zikula Core Framework
GNU Lesser General Public License v3.0
237 stars 69 forks source link

Weird problem with username validation "The user name you entered is reserved. It cannot be used." #2942

Closed Kaik closed 7 years ago

Kaik commented 7 years ago
Q A
Zikula Version 1.4.3
PHP Version 5.6

In ZikulaIntercomModule which I'm refactoring, I have a new message form and there is a recipient field. https://github.com/Kaik/InterCom/blob/master/Form/Type/MessageType.php#L30 When I type admin as recipient and press send it shows me this error above that field The user name you entered is reserved. It cannot be used. Since I did not implement this kind of validation I have looked in core and it is present here https://github.com/zikula/core/blob/02ec0226f7d18196d2cab099f2ab2b26327b1fdd/src/system/ZAuthModule/Validator/Constraints/ValidUnameValidator.php#L84 and here https://github.com/zikula/core/blob/02ec0226f7d18196d2cab099f2ab2b26327b1fdd/src/system/UsersModule/Validator/Constraints/ValidUnameValidator.php#L84

Expected behavior

UsersModule and ZAuth module username validation to work only in those modules.

Actual behavior

UsersModule and ZAuth module username validation works globally?

Steps to reproduce

Install master from here https://github.com/Kaik/InterCom and send a message to admin. Of course, admin username needs to be restricted in UsersModule settings

This is very wired... @craigh

craigh commented 7 years ago

UsersModule and ZAuth module username validation works globally?

as I understand it, this is impossible.

craigh commented 7 years ago

I'm guessing here, but here's what I will guess is happening:

The validation is built into the UserEntity (see this). So in order to get the error you must be (1) validating the entity and (2) attempting to persist it (I think). So, you might check some cascade behavior in your other entities. Or take a look at your logic (which I have not done). Obviously it should not be getting validated or persisted 😄 so the question is why is it doing that? You can also look at the search methods (here and here in the users module to see how it is different than yours.

IMO this is not a core problem.

Kaik commented 7 years ago

Hmm I will test it, but I would not be so sure that it is not core. Because I remember testing it before 1.4.1 or even 1.4.2 well about 6-9 months ago and it was working. There is only one entity - messages with

    /**
     * The recipient uid
     *     
     * @ORM\ManyToOne(targetEntity="Zikula\Module\UsersModule\Entity\UserEntity")
     * @ORM\JoinColumn(name="recipient", referencedColumnName="uid")
     */
    private $recipient;

What I would have to do to possibly persist user entity and trigger validation using ManyToOne annotation which is unidirectional by the way... well I will check logic There are things I suspect could be a reason like ModelTransformer name that I use gets overridden somewhere and interfere with this validation, but for me most probable is that this validation somehow works globally... anyway I will trace it

craigh commented 7 years ago

despite the fact that I did so in Dizkus, I can no longer recommend non-core modules using a UserEntity as a relation as you doing here. I would recommend storing only the UID.

Kaik commented 7 years ago

What wonders me more is that MessageEntity has two user fields sender and recipient and only recipient gives this error, sending message from admin account to other account works only when I try to send it to admin account triggers this error.

Kaik commented 7 years ago

What you mean should I store it as integer not as relation? I do not understand why?

craigh commented 7 years ago

sending message from admin account to other account works

then there is a logic error somewhere.

What you mean should I store it as integer not as relation?

/**
 * The recipient uid
 *     
 * @ORM\Column(type="integer")
 */
private $recipient;
Kaik commented 7 years ago

So what you are saying is to leave the idea of related objects on which Doctrine is based on and use integer instead. So module developers to populate user object will have to either use join in the repository or I do not know iterate over objects array to get user object by id if they need it? This is so bad... I do not understand why it is wrong to use a relation to UserEntity this is like back to the stone age. User object for any entity is the most wanted one please check any module table and there is at least one user field. I really would like to know what is the reason behind this. It is like you would say let's not use relations at all and do everything the old way using id's and joins... we could stick with DBUtil to do that.

craigh commented 7 years ago

holy smokes. overreact much? take a breath.

what I'm saying is that if your problem works sometimes and not others, then it is a logic problem.

cmfcmf commented 7 years ago

holy smokes. overreact much? take a breath.

I am tempted to agree with @Kaik though.

Kaik commented 7 years ago

Lol I disagree that it is a logic problem, well it might be but not in IntercomModule. What I pointed out is that it lays somewhere in form layer. It does not work only with model transformer (where I type username and then convert it to id, recipient field is done this way) while sender is internal by uid and it works. I will be back soon I'm writing from phone

craigh commented 7 years ago

Please test your issue against latest core changes.

Kaik commented 7 years ago

I will tomorrow. I'm making changes in my office (new bench) and computer on which I'm working is... well it is on the floor right now in pieces. I have seen changes you have made and I do not think it will help as it works only for admins. What would help is to restrict validation only to entity creation, but it would be kind of wrong as well.

I'm very surprised that this error is triggered anyway - as I'm not changing user entity only "selecting it" in many to one relation. I was looking for any similar usage (where there is DataTransformer and Validation Constraint) but I could not find any.

I had no time to investigate it further I was really busy last month. What I have found some time ago is that there are two kinds of transformers Model and View I'm using model transformer now. It used to work in core 1.4.2, but it might be that I need to use View transformer instead of Model both are very similar but are woking on different levels and it might work I need to test it. http://symfony.com/doc/current/form/data_transformers.html#about-model-and-view-transformers

craigh commented 7 years ago

I do not think it will help as it works only for admins.

there are other changes and therefore my request.

Kaik commented 7 years ago

Ok I will check it tomorrow.

Kaik commented 7 years ago

This is still the same. Core at 4961ffa... I have tested both addModelTransformer and addViewTransformer screenshot from 2016-08-14 11-02-47

I have even disabled "saving" so I can see where error occurs and it occurs during validation.

   public function newMessageAction(Request $request) {
        // Permission check
        if (!$this->get('zikula_intercom_module.access_manager')->hasPermission()) {
            throw new AccessDeniedException();
        }
        $options = ['isXmlHttpRequest' => $request->isXmlHttpRequest()];
        $message = $this->get('zikula_intercom_module.manager.message')->create();
        $form = $this->createForm('messageform', $message, $options);
        $form->handleRequest($request);

        if ($form->isValid()) {
//            $this->get('zikula_intercom_module.manager.message')->setNewData($form->getData());
//            $this->get('zikula_intercom_module.manager.message')->send();
//            if ($request->isXmlHttpRequest()) {
//                return new JsonResponse(array('status' => true));
//            } else {
//                $request->getSession()
//                        ->getFlashBag()
//                        ->add('status', "Message send!");
//                return $this->redirect($this->generateUrl('zikulaintercommodule_inbox_view'));
//            }
        }

        if ($request->isXmlHttpRequest()) {
           return new JsonResponse(array('status' => true, 'html' => $this->renderView('ZikulaIntercomModule:Message:form.html.twig', array(
                    'form' => $form->createView(),
                    'message' => $message,
                    'settings' => $this->getVars()
        )))); 
        }
        $request->attributes->set('_legacy', true); // forces template to render inside old theme
        return $this->render('ZikulaIntercomModule:Message:new.html.twig', array(
                    'form' => $form->createView(),
                    'message' => $message,
                    'settings' => $this->getVars()
        ));
    } 
    public function buildForm(FormBuilderInterface $builder, array $options) {

        $builder->setMethod('POST')
                ->add($builder->create('recipient', 'text', ['invalid_message' => $this->__('User not found.'),'attr' => ['class' => 'author_search']])
                        ->addModelTransformer(new UserToIdTransformer($this->entityManager)))
                ->add($builder->create('groups', 'text', ['mapped' => false,'required' => false,'invalid_message' => $this->__('Group not found.'), 'attr' => ['class' => 'author_search']])
                        ->addModelTransformer(new GroupToIdTransformer($this->entityManager)))
                ->add('subject', 'text', [
                    'required' => false
                ])
                ->add('text', 'textarea', [
                    'required' => true, 'attr' => [
                        'class' => 'tinymce'
                    ]
                ]);

        if ($options['isXmlHttpRequest'] == false) {
            $builder->add('save', 'submit', [
                'label' => $this->__('Send'),
                'attr' => [
                    'class' => 'btn-success'
                ]
            ]);
        }       

    }
Kaik commented 7 years ago

Here I have found one information regarding this behaviour, well it is opposite situation but looks like there is answer what is causing validation to occur before transformation. http://www.scriptscoop.me/t/fe4a2b8398a6/forms-symfony2-form-field-constraints-validation-before-data-transformer.html http://qaoverflow.com/question/symfony2-form-field-constraints-validation-before-data-transformer/

Funny thing is that this is me, in a nutshell, most of the questions regarding data transformation and validation constraints ask what this issue is about - they ask how can they make validation occur before transformation while I just want it to work as default :)

craigh commented 7 years ago

can you mail me the module so I can see it for myself?

Kaik commented 7 years ago

Yes, I will do it today later one I'm in the middle of working on it and some parts are disabled etc so I need to test if it installs properly.

Btw, I just wanted to check if adding constraint validation to form would work I have checked forms and I was surprised to find out that it is already implemented there, so I went to UserEntity and removed this line https://github.com/zikula/core/blob/1.4/src/system/UsersModule/Entity/UserEntity.php#L47 and everything works both validation in registration form and sending message to admin.

I will check if module installs properly and will email it to you today.

craigh commented 7 years ago

issue was determined to be not-core.

Kaik commented 7 years ago

Yes, logic problem in InterCom, my fault.