Flag of Ukraine
SymfonyCasts stands united with the people of Ukraine
TRACK

Symfony 5 >

COURSE

Contributing back to Symfony!

Coding a new Feature

Video not working?

It looks like your browser may not support the H264 codec. If you're using Linux, try a different browser or try installing the gstreamer0.10-ffmpeg gstreamer0.10-plugins-good packages.

Thanks! This saves us from needing to use Flash or encode videos in multiple formats. And that let's us get back to making more videos :). But as always, please feel free to message us.
Previous Chapter

Next Chapter
Edit on GitHub

We've just created a new branch based off of Symfony's master branch. And now, we're ready to create the amazing new TargetPathHelper class. But... where should it live? It's related to Security... which means it could live in the Security component or SecurityBundle.

Components Versus Bundles

As a general rule, most code should live in a component so that it's reusable even outside of the framework. But, sometimes, you'll write code that's really integrated with the framework. That code will live in the bundle. My best advice... don't over-think it: it usually becomes pretty obvious if you put something in the wrong spot.

Press Shift+Shift and search for a file that's closely related to our new feature: TargetPathTrait. Ok, this lives in the Security component. I'll double click on the directory to move there. At first, it seems like TargetPathHelper should live right here. And that's where I would put it at first. I say "at first" because, if you started coding, you'd notice a problem.

What problem? This new class will ultimately use the FirewallMap class internally to do its work. There are two FirwallMap classes: one lives in the Security component, and the other lives in SecurityBundle. After digging a little bit, you'd find out that we will need to use the one from SecurityBundle.

And here's why that's important: a class in a component can depend on classes from other components. But, it can never depend on a class from a bundle. Because our new TargetPathHelper needs a class from SecurityBundle, it can't live in the component: it must live in the bundle.

If you get this wrong, no big deal: someone will help you out on your pull request.

Go find SecurityBundle and look inside Security. Hey! Here are the FirewallMap and FirewallConfig classes we'll be using! That's a good sign! Create the new PHP class: TargetPathHelper. Add our first public function, how about just savePath() with a string $uri argument:

12 lines symfony/src/Symfony/Bundle/SecurityBundle/Security/TargetPathHelper.php
... lines 1 - 2
namespace Symfony\Bundle\SecurityBundle\Security;
class TargetPathHelper
{
public function savePath(string $uri)
{
}
}

Symfony 4.0 and above requires PHP 7.1, so you should use scalar type-hints and return types. But, Symfony does not use the void return type.

All About PHPDoc

Because this is a public function, we should add some PHPDoc to describe it. Add a clear, but short description above this:

26 lines symfony/src/Symfony/Bundle/SecurityBundle/Security/TargetPathHelper.php
... lines 1 - 13
class TargetPathHelper
{
/**
* Sets the target path the user should be redirected to after authentication.
*
* @param string $uri The URI to set as the target path
*/
public function savePath(string $uri)
{
}
}

Actually, Symfony does not have a lot of PHPDoc... which might seem weird at first. The reason is that we don't want to maintain too much documentation inside the code - we use a separate repository for documentation.

Oh, and, thanks to the string type-hint, the @param documentation is totally redundant and should be removed... unless there's some valuable extra info that you want to say about it. I'll keep it and add some extra notes... even though it doesn't add a lot of extra context:

26 lines symfony/src/Symfony/Bundle/SecurityBundle/Security/TargetPathHelper.php
... lines 1 - 13
class TargetPathHelper
{
/**
... lines 17 - 18
* @param string $uri The URI to set as the target path
*/
public function savePath(string $uri)
{
}
}

Also every PHP file in Symfony should have a copyright header on top. Grab that from another file and paste it here:

26 lines symfony/src/Symfony/Bundle/SecurityBundle/Security/TargetPathHelper.php
... lines 1 - 2
/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/
namespace Symfony\Bundle\SecurityBundle\Security;
class TargetPathHelper
{
... lines 16 - 24
}

Don't worry too much about these details: it's easy to add them later if you forget.

Injecting the Services we Need

To make life nicer, use TargetPathTrait on top of the class:

30 lines symfony/src/Symfony/Bundle/SecurityBundle/Security/TargetPathHelper.php
... lines 1 - 13
use Symfony\Component\Security\Http\Util\TargetPathTrait;
class TargetPathHelper
{
use TargetPathTrait;
... lines 19 - 28
}

Then all we need to do is say $this->saveTargetPath():

30 lines symfony/src/Symfony/Bundle/SecurityBundle/Security/TargetPathHelper.php
... lines 1 - 15
class TargetPathHelper
{
use TargetPathTrait;
... lines 19 - 24
public function savePath(string $uri)
{
$this->saveTargetPath();
}
}

But... hmm... this needs 3 arguments: the session, provider key - which is the firewall name - and the URI. We know that we can get the firewall name by using the FirewallMap service. Let's add some constructor arguments: SessionInterface $session and FirewallMap - the one from SecurityBundle - $firewallMap:

46 lines symfony/src/Symfony/Bundle/SecurityBundle/Security/TargetPathHelper.php
... lines 1 - 13
use Symfony\Component\HttpFoundation\Session\SessionInterface;
... lines 15 - 16
class TargetPathHelper
{
... lines 19 - 24
public function __construct(SessionInterface $session, FirewallMap $firewallMap)
{
... lines 27 - 28
}
... lines 30 - 44
}

I'll press Alt+Enter and select initialize fields to create those properties and set them:

46 lines symfony/src/Symfony/Bundle/SecurityBundle/Security/TargetPathHelper.php
... lines 1 - 16
class TargetPathHelper
{
... lines 19 - 20
private $session;
private $firewallMap;
public function __construct(SessionInterface $session, FirewallMap $firewallMap)
{
$this->session = $session;
$this->firewallMap = $firewallMap;
}
... lines 30 - 44
}

Make sure to remove the PHPDoc above each property: this is redundant thanks to the constructor type-hints.

To calculate the provider key, create a new private function: getProviderKey() that will return a string. For now, just put a TODO:

46 lines symfony/src/Symfony/Bundle/SecurityBundle/Security/TargetPathHelper.php
... lines 1 - 16
class TargetPathHelper
{
... lines 19 - 40
private function getProviderKey(): string
{
// TODO
}
}

Back up in setTargetPath(), pass $this->session, $this->getProviderKey() and the $uri:

46 lines symfony/src/Symfony/Bundle/SecurityBundle/Security/TargetPathHelper.php
... lines 1 - 16
class TargetPathHelper
{
... lines 19 - 35
public function savePath(string $uri)
{
$this->saveTargetPath($this->session, $this->getProviderKey(), $uri);
}
... lines 40 - 44
}

Awesome! Look back at getProviderKey():

46 lines symfony/src/Symfony/Bundle/SecurityBundle/Security/TargetPathHelper.php
... lines 1 - 16
class TargetPathHelper
{
... lines 19 - 40
private function getProviderKey(): string
{
// TODO
}
}

I didn't add any PHPDoc to this function, but that's not because I'm lazy. Or... not entirely because I'm lazy. Really, it's for two reasons. First, this is a private function, and those are typically not documented inside Symfony. And second, we already have the return type - no reason to duplicate it!

Lets finish this function. To get the firewall name, we need to use the FirewallMap, call getFirewallConfig() and pass it the request. Ok: $firewallConfig = $this->firewallMap->getFirewallConfig(). But, hmm... we don't have the Request object! No problem: add a third constructor arg: RequestStack $requestStack. I'll hit Alt+Enter again to create that property and set it. Clean off the PHPDoc, then head back down:

56 lines symfony/src/Symfony/Bundle/SecurityBundle/Security/TargetPathHelper.php
... lines 1 - 13
use Symfony\Component\HttpFoundation\RequestStack;
... lines 15 - 17
class TargetPathHelper
{
... lines 20 - 25
private $requestStack;
public function __construct(SessionInterface $session, FirewallMap $firewallMap, RequestStack $requestStack)
{
... lines 30 - 31
$this->requestStack = $requestStack;
}
... lines 34 - 54
}

Normally, when you use RequestStack, you call its getCurrentRequest() method to get the request. But, in this case, I'm going to use another method: $this->requestStack->getMasterRequest():

56 lines symfony/src/Symfony/Bundle/SecurityBundle/Security/TargetPathHelper.php
... lines 1 - 17
class TargetPathHelper
{
... lines 20 - 44
private function getProviderKey(): string
{
$firewallConfig = $this->firewallMap->getFirewallConfig($this->requestStack->getMasterRequest());
... lines 48 - 53
}
}

I'm not 100% sure that this is correct. The whole topic of requests and sub-requests is pretty complex. But, basically, Symfony's security firewall only operates on the outer, "master" request. So, to find the active firewall, that's what we should use. If I'm wrong, hopefully someone will tell me on the pull request.

Next, if you look at the getFirewallConfig() method, it's possible that this will return null. Code for that: if null === $firewallConfig:

56 lines symfony/src/Symfony/Bundle/SecurityBundle/Security/TargetPathHelper.php
... lines 1 - 17
class TargetPathHelper
{
... lines 20 - 44
private function getProviderKey(): string
{
$firewallConfig = $this->firewallMap->getFirewallConfig($this->requestStack->getMasterRequest());
if (null === $firewallConfig) {
... line 50
}
... lines 52 - 53
}
}

This is another Symfony coding convention: we use Yoda conditionals!

Mysterious, Symfony's coding conventions are. Herh herh herh.

But hey! If the force isn't strong with you today, don't worry: Symfony has a magic way of fixing coding convention problems that I'll show you later.

If there is no firewall config for some reason, throw a new LogicException with as clear a message as possible:

56 lines symfony/src/Symfony/Bundle/SecurityBundle/Security/TargetPathHelper.php
... lines 1 - 17
class TargetPathHelper
{
... lines 20 - 44
private function getProviderKey(): string
{
$firewallConfig = $this->firewallMap->getFirewallConfig($this->requestStack->getMasterRequest());
if (null === $firewallConfig) {
throw new \LogicException('Could not find firewall config for the current request');
}
... lines 52 - 53
}
}

Why a LogicException? Well, it seems to make sense - something went wrong... logically. And usually, the exact exception class won't matter. If it does matter, someone will tell you when reviewing your PR.

Finally, at the bottom, return $firewallConfig->getName():

56 lines symfony/src/Symfony/Bundle/SecurityBundle/Security/TargetPathHelper.php
... lines 1 - 17
class TargetPathHelper
{
... lines 20 - 44
private function getProviderKey(): string
{
$firewallConfig = $this->firewallMap->getFirewallConfig($this->requestStack->getMasterRequest());
if (null === $firewallConfig) {
throw new \LogicException('Could not find firewall config for the current request');
}
return $firewallConfig->getName();
}
}

That should be it!

While we're here, let's add one more function: getPath() that will return a string. Inside, return $this->getTargetPath() with $this->session and $this->getProviderKey():

64 lines symfony/src/Symfony/Bundle/SecurityBundle/Security/TargetPathHelper.php
... lines 1 - 17
class TargetPathHelper
{
... lines 20 - 47
public function getPath(): string
{
return $this->getTargetPath($this->session, $this->getProviderKey());
}
... lines 52 - 62
}

This time, I will add some PHPDoc. I don't need @return - that's redundant - but I will add a description about what this method does:

64 lines symfony/src/Symfony/Bundle/SecurityBundle/Security/TargetPathHelper.php
... lines 1 - 17
class TargetPathHelper
{
... lines 20 - 44
/**
* Returns the URL (if any) the user visited that forced them to login.
*/
public function getPath(): string
{
return $this->getTargetPath($this->session, $this->getProviderKey());
}
... lines 52 - 62
}

Making the Class final

And... we're done! Yea, we still need to write a test & add some config to register this as a service: we'll do that next. But, this class should work!

However... I am going to make one, ahem, final change: add final:

64 lines symfony/src/Symfony/Bundle/SecurityBundle/Security/TargetPathHelper.php
... lines 1 - 17
final class TargetPathHelper
{
... lines 20 - 62
}

Making this class final means that nobody is allowed to subclass it. Why are we doing this? Because, in the future, if the class is final, it will be easier to make changes to it without breaking backwards compatibility. Basically, if you allow a class to be sub-classed, you have to be a bit more careful when making certain changes. Making classes final is a good "default" for new Symfony classes.

Of course, if there is a legitimate use-case for some to sub-class this, then you don't need to make it final. But, while we can easily remove final later, we can't add final in the future, at least not without jumping through a few extra hoops to avoid breaking backwards compatibility.

Ok, let's add some service config & a test!

Leave a comment!

Most Useful Newest Oldest
5
Login or Register to join the conversation
Default user avatar
Default user avatar Alexane | posted 4 years ago

Is is a bug?

In the last code block, when the page is initially loaded, we have lines 20-62 hidden. Then you click arrows and have lines 20-41 hidden. Then you click arrows again and have no more lines hidden. But that is not true! Line 20 is still hidden, but no arrows - one has to click the square on the top of the code block.

How did I found this bug? I wasn't into context of the tutorial because I only needed to check one thing out and I didn't remember that you were using TargetPathTrait in your Helper. I just needed to check something in the section "Making the Class final". So I scrolled down to investigate this one code block and I couldn't figure out where the hell are saveTargetPath() and getTargetPath() coming from. Maybe he used a trait? No, there is no trait. And after few minutes I recognized the line 20 was missing.

Reply
Victor Avatar

Hey Alexane,

Ah, we're sorry about this! And yeah, good catch, it looks like a bug! Thanks for reporting it, we'll take a look at it soon. For now, as a workaround, you can expand ALL lines by clicking on squared icon in the left top corner of the code block.

Cheers!

Reply
Default user avatar

Ryan says: "Oh, and, thanks to the string type-hint, the @param documentation is totally redundant and should be removed."

Isn't it against Symfony Coding Standards which say: "Add PHPDoc blocks for all classes, methods, and functions (though you may be asked to remove PHPDoc that do not add value);"

How shout I interpret this?

Reply
weaverryan Avatar

Hey Marcin!

Ah, you have a close eye - nice work :). I think what the docs are saying and what I'm saying are 2 ways of "trying to explain the same thing". When I say:

the @param documentation is totally redundant and should be removed

I didn't meant to imply that all @param documentation is redundant, just that specific @param. Especially with scalar type-hints, if you type-hint an argument, and the name of the argument makes it very obvious what the purpose of that argument is, then adding something like @param string $name does not "add value" (as the Symfony docs say). However, if the argument truly needs some more explanation - e.g. @param string $providerKey The name of the firewall that is currently being used - then the @param DOES add value and should be included.

The point is: question whether or not something adds value or not. We typically think that all documentation adds some value. But because Symfony is such a large codebase, docs also add extra overhead (e.g. future pull requests to do simple things like re-word those docs, or add punctuation) - so the docs really need to add value.

I hope that clarifies - very good question!

Cheers!

Reply
Default user avatar

Thanks Ryan!

After few days of having this standard in back of my head, I think I've misunderstood (however my interpretation makes sense when to think about it) the word "though" in the context of the sentence. I thought I should add phpdocs THOUGH I may be asked (by something, eg. IDE or someone, eg. developer who doesn't follow standards) to remove the ones that do not add any value (so ignore those questions and add dockblocs anyway).
Now I can see I should add phpdocs HOWEVER I may be asked (eg. by Symfony Team member) to remove the ones that do not add any value (which leads to not to add this docsblocks at first so nobody should ask me to remove it after I make pull request).

Sometimes, the same sentence means something different in different days :)

Reply
Cat in space

"Houston: no signs of life"
Start the conversation!

The concepts in this tutorial work great for Symfony 5!
Symfony 4
Edit on GitHub
Previous Chapter
Next Chapter
userVoice