trjhtr

This is the code I have written for one of my API routes. I feel like my controller is too big and the function does too much. What can I do to make my adhere to SOLID principles and be more maintainable/readable?

/**
 * @Route("/registration", name="registration")
 * @Method("POST")
 * @throws UniqueConstraintViolationException
 * @param Request $request
 * @return JsonResponse
 */
public function registrationAction(Request $request, UserPasswordEncoderInterface $encoder)

 // Build base response
 $response = [
 "success" => false,
 "message" => "",
 "user_id" => null,
 "errors" => 
 ];

 // Put blanks for keys not present in the request
 // This is so we can validate this using Symfony's validator class
 foreach(self::REQUIRED_KEYS as $key) 
 if (!array_key_exists($key, $request->request->all())) 
 $request->request->set($key, "");
 
 

 // Get validator and build array of constraints
 $validator = Validation::createValidator();
 $constraint = new AssertCollection([
 "username" => new AssertNotBlank(['message' => "Username is required."]),
 "email" => [
 new AssertEmail(["message" => "Email must be a valid email address."]),
 new AssertNotBlank(["message" => "Email is required."])
 ],
 "address1" => new AssertNotBlank(['message' => "Address1 is required."]),
 "city" => new AssertNotBlank(['message' => "City is required."]),
 "state" => new AssertNotBlank(['message' => "State is required."]),
 "zip" => new AssertNotBlank(['message' => "Zip is required."]),
 ]);
 $constraint->allowExtraFields = true;

 $errors = $validator->validate($request->request->all(), $constraint);

 // If there are errors, return the errors as a response
 // If there are no errors, register the user and return the ID
 if (count($errors) > 0) 
 foreach($errors as $e) $response['errors'] = $e->getMessage(); 
 $response['message'] = "Submitted user failed validation.";
 else 
 $user = new User($request->request->all());
 $encodedPassword = $encoder->encodePassword($user, 'password');
 $user->setPassword($encodedPassword);

 $em = $this->getDoctrine()->getManager();
 try 
 $user->setEnabled(true);
 $em->persist($user);
 $em->flush();
 $response['success'] = true;
 $response['user_id'] = $user->getId();
 catch (UniqueConstraintViolationException $e) 
 preg_match('/(?<=Duplicate entry ')[^']*/', $e->getMessage(), $matches);
 $response['message'] = "Unique constraint violation exception";
 $response['errors'] = sprintf('%s already exists in the database.', $matches[0]);
 
 

 return new JsonResponse($response);

My initial hunch is that I should put this into a service and break it up. But it seems like I would just be copying and pasting this code into class.

I feel like my controller is too big and the function does too much.

I think you're right there. Your validation rules shouldn't be in the controller, but in separate classes. That makes re-using them much easier. I

My initial hunch is that I should put this into a service and break it
up. But it seems like I would just be copying and pasting this code
into class.

You're right here. Just moving code from your controller to a service makes no sense.

Why aren't you using Symfony Forms? It does exactly the job you're doing here: take the Request and some rules and tell you if the request matches your validation rules.

You can create an object (a Doctrine entity or a Data Transfer Object, DTO), add the constraints to it by using annotations.

See this to examples from https://stovepipe.systems/post/avoiding-entities-in-forms:

Your DTO:

class ChangeUsernameData

 /** 
 * @SomeCustomAssertUnique(entity="AppEntityAuthentication", field="username")
 * @AssertLength(5, 16)
 **/
 public $newUsername;

Your controller:

$changeUsername = new ChangeUsernameData();
 $form = $this->formFactory->create(ChangeUsernameType::class, $changeUsername);
 $form->handleRequest($request);

 if ($form->isSubmitted() && $form->isValid()) 
 $authentication->setUsername($changeUsername->newUsername);
 $this->em->flush($authentication);

 // redirect