Skip to content
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

Refactor routing #1178

Open
wants to merge 21 commits into
base: master
from

Conversation

@tvdijen
Copy link
Member

commented Aug 3, 2019

@tvdijen tvdijen changed the title Refactor routing WIP: Refactor routing Aug 3, 2019

@tvdijen

This comment was marked as resolved.

Copy link
Member Author

commented Aug 3, 2019

I've managed to rebase this old branch against master.. Went pretty smoothly, actually!
But running tests with with branch I'm running into:

Aug 3 18:15:18 webapp-1 simplesamlphp-idp[2088]: 3 [a47efda04c] Caused by: InvalidArgumentException: Controller "SimpleSAML\Module\core\Controller\LoginController" cannot be fetched from the container because it is private. Did you forget to tag the service with "controller.service_arguments"?

@sgomez Would you be able to help with this?

@tvdijen tvdijen added this to the 1.18 milestone Aug 3, 2019

@tvdijen tvdijen added the help wanted label Aug 3, 2019

@tvdijen tvdijen force-pushed the refactor-routing-rebased branch 3 times, most recently from 250797d to 6594d55 Aug 4, 2019

@tvdijen tvdijen removed the help wanted label Aug 5, 2019

@tvdijen tvdijen changed the title WIP: Refactor routing Refactor routing Aug 5, 2019

@tvdijen tvdijen requested a review from jaimeperez Aug 5, 2019

@tvdijen
Copy link
Member Author

left a comment

A few comments I have myself on this code:

  • I'm not sure cache should go into the tempdir, but we don't really have a cache-dir unless we use the same cache-dir we use for Twig-templates.. In that case, we need a default/fallback in case it is not set.
  • We need to update the release-procedure to update the constants in the Kernel-class.. Can we improve this so we have less settings to update on a new release?
  • I'm not happy with the location of the configuration files.. I feel they shouldn't go inside the lib/ directory, but somewhere in the root-directory
  • One thing I've noticed is that things break if the log/cache-dirs are not writeable by Apache/PHP.. I don't log to files, but directly to syslog, so it shouldn't necessarily break. This may not be avoidable because it's Symfony that chokes..
Show resolved Hide resolved lib/SimpleSAML/Kernel.php Outdated

@tvdijen tvdijen force-pushed the refactor-routing-rebased branch 4 times, most recently from 6e85845 to a3dc852 Aug 8, 2019

@tvdijen tvdijen force-pushed the refactor-routing-rebased branch 3 times, most recently from 2d4a270 to 8540474 Aug 8, 2019

@tvdijen

This comment was marked as resolved.

Copy link
Member Author

commented Aug 9, 2019

Something that also needs fixing is the fact that /module.php/admin doesn't resolve to the admin main-page right now.. It doesn't recognize the / route in the admin-module but instead tries to find a /admin route in the base routing file..
On the SimpleSAML-end we are loading the correct routes, it's just that on the Symfony-end they are not matched the way we want it.

I was able to fix it so that admin becomes admin/ and that admin/federation/ becomes admin/federation..

@tvdijen tvdijen force-pushed the refactor-routing-rebased branch 7 times, most recently from df51aea to 5d81359 Aug 9, 2019

@tvdijen tvdijen force-pushed the refactor-routing-rebased branch 2 times, most recently from 6d93ec6 to 2d7943a Aug 10, 2019

@jaimeperez
Copy link
Member

left a comment

This looks good, great work! 👍

I have plenty of doubts though. The most important one would be how to use all of this. I'm missing documentation on how to write modules, mainly how to define routes and controllers for them, how to define services (and what are those useful for), etc. Basically, all the things needed to keep current functionality. Including how to make it work after installing the module (should this be done automatically in the composer-module-installer?).

I'm also particularly interested in replacing the hooks functionality, and if I recall correctly from previous conversations, this would be possible with services. How could we use that with this setup? Would it be possible to have something analogous to what we have today, where we call a hook, and then all hooks defined by enabled modules are executed? Asking for a friend 🤣

@sgomez maybe you can help us out a bit? @bendikrb do you have any suggestions?

Show resolved Hide resolved lib/SimpleSAML/XHTML/Template.php Outdated
Show resolved Hide resolved modules/admin/lib/Controller/ConfigController.php Outdated
@@ -0,0 +1,24 @@
admin-main:

This comment has been minimized.

Copy link
@jaimeperez

jaimeperez Aug 13, 2019

Member

Is it a Symfony convention to have routing/routes and a routing/services directories? If not, I think it makes it slightly more complicated for users having their own modules. I think I would prefer to have these configuration files directly in the root directory of the module...

This comment has been minimized.

Copy link
@tvdijen

tvdijen Aug 13, 2019

Author Member

We load them in different ways and I don't think we have a way to distinguish them...
here and here
We could lose the routing prefix in the path though.. Most modules will only have routes, not services I think...

This comment has been minimized.

Copy link
@bendikrb

bendikrb Aug 15, 2019

Collaborator

Again with the "conventions", in regards to files/structure; I would strongly suggest that we stick to the way symfony-components organizes these resources (especially for new stuff, for which backwards compability isn't an issue);
Resources/config/services.yml
Resources/config/routing.yml
Modules should also be extending Extension, and use processConfiguration (happens during building of the service container) - but I guess we need to draw the line at some point, in regards to symfonizing the application...

This comment has been minimized.

Copy link
@tvdijen

tvdijen Aug 15, 2019

Author Member

Again, I don't really care for conventions that don't add anything.. We can put those files anywhere we want. Most Symfony-apps I know put those files in app/config..

Symfonizing is not necessarily a bad thing, as we start using more of their stuff. I'd actually like to expand this and start using dbal/doctrine/ldap as well for abstraction..

This comment has been minimized.

Copy link
@bendikrb

bendikrb Aug 15, 2019

Collaborator

Again, I don't really care for conventions that don't add anything.. We can put those files anywhere we want.

I agree, except with the fact that these conventions don't add anything, because they do; consistency. Which I find quite important, especially when having contributors all over the place writing code, either in core or contrib modules.
As to app/config - yes, that's where the application wide configuration is. The modules, or extensions if you may, put them in Resources/config.

I think the main reason for why I find reading code of Symfony projects so pleasant, is because there is consistency in where things are, and how they're named.
Not to mention the framework support from IDE's such as JetBrains - which wouldn't work (atleast not out of the box) if things are named/placed arbitrary in the codebase.

Not trying to start a flame war here, I just thought I'd point out a couple of aspects I see about this :)

Show resolved Hide resolved modules/core/lib/Controller/RedirectionController.php Outdated
Show resolved Hide resolved modules/core/lib/Controller/RedirectionController.php Outdated
Show resolved Hide resolved modules/core/templates/login.twig
@@ -0,0 +1,72 @@
services:

This comment has been minimized.

Copy link
@jaimeperez

jaimeperez Aug 13, 2019

Member

It's very confusing to have services defined in a file named routing/services/routing.yml.

Also, is .yml the proper extension or should it be .yaml?

This comment has been minimized.

Copy link
@tvdijen

tvdijen Aug 13, 2019

Author Member

It can be both, but all applications I know use .yml... We used both at some point, so I picked one for consistency..
You have a valid point about the confusion, but I would have to figure out what the consequences are if we rename it

This comment has been minimized.

Copy link
@bendikrb

bendikrb Aug 15, 2019

Collaborator

I'm also particularly interested in replacing the hooks functionality, and if I recall correctly from previous conversations, this would be possible with services. How could we use that with this setup? Would it be possible to have something analogous to what we have today, where we call a hook, and then all hooks defined by enabled modules are executed? Asking for a friend rofl

@sgomez maybe you can help us out a bit? @bendikrb do you have any suggestions?

I would recommend to implement EventDispatcher, and have the modules subscribe to the various events (i.e. "hooks").
I'm looking in to how to best do this now - while also keeping it somewhat backwards compatible. This ties together with having a service container at the core too - as far as I can grasp.

class: Symfony\Component\Routing\Loader\XmlFileLoader
arguments:
- '@file_locator'
tags: ['routing.loader']

This comment has been minimized.

Copy link
@jaimeperez

jaimeperez Aug 13, 2019

Member

I assume this is not necessary if we're only allowing YAML config files, right?

This comment has been minimized.

Copy link
@tvdijen

tvdijen Aug 13, 2019

Author Member

Why would you want to restrict this? Some module may have a need for flexible PHP config-files..
What we allow is set here

@@ -0,0 +1,33 @@
services:

This comment has been minimized.

Copy link
@jaimeperez

jaimeperez Aug 13, 2019

Member

So... even more confusing? 🤔

factory: ['SimpleSAML\Session', 'getSessionFromRequest']

SimpleSAML\Auth\AuthenticationFactory:
class: SimpleSAML\Auth\AuthenticationFactory

This comment has been minimized.

Copy link
@jaimeperez

jaimeperez Aug 13, 2019

Member

This seems to be used only in tests. Should we use it in the controllers as well?

This comment has been minimized.

Copy link
@tvdijen

tvdijen Aug 13, 2019

Author Member

🤷‍♂ Your guess is as good as mine

@tvdijen tvdijen force-pushed the refactor-routing-rebased branch 3 times, most recently from bab6d06 to 98f6212 Aug 13, 2019

@tvdijen

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

As for documentation I think we just have to come up with a 2.0 version of https://github.com/simplesamlphp/simplesamlphp-module-examplecomposer

@bendikrb
Copy link
Collaborator

left a comment

I believe the "Controller" suffix is a naming convention in Symfony that we should stick to.
It can quickly become cluttered too, with such generic names as "Config" - especially if you need to use for instance "Config" the model and "Config" the controller in the same context.

@tvdijen tvdijen force-pushed the refactor-routing-rebased branch 2 times, most recently from 1c81ac0 to 1f56930 Aug 15, 2019

@tvdijen tvdijen force-pushed the refactor-routing-rebased branch from 1f56930 to 0c91640 Aug 15, 2019

@tvdijen

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2019

I believe the "Controller" suffix is a naming convention in Symfony that we should stick to.
It can quickly become cluttered too, with such generic names as "Config" - especially if you need to use for instance "Config" the model and "Config" the controller in the same context.

I've had this discussion with Jaime a couple of days ago and it all depends on how you're namespacing things.. If you move your namespaces one level up you get Model/Config and Controller/Config .. I don't care much for conventions that don't really add anything..

@bendikrb

This comment has been minimized.

Copy link
Collaborator

commented Aug 15, 2019

If you move your namespaces one level up you get Model/Config and Controller/Config .. I don't care much for conventions that don't really add anything..

Yes, technically its of course a non issue, and separating like this is consistent with the conventions of Symfony aswell. The point I was trying to make is that then we're likely to get something like..

use SimpleSAML\Module\user\Controller\User as UserController;
use SimpleSAML\Module\user\Model\User as UserModel;
use SimpleSAML\Module\user\Event\User as UserEvent;

.. at which point I do think that this naming convention provides value.
In unit testing we'll get a Test suffix on these aswell - so then it quickly can get untidy, in my opinion.

@tvdijen

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2019

In our case we will only have Controllers.. The only Model we have is the Template-class and our views are Twig-templates..
Personally. if I take your example, I would use SimpleSAML\Module\user\Controller; and then call new Controller\Config();.. That leaves enough significance to distinguish classes with the same name.. Just grep the codebase, you will not find more than 1 or 2 'as ClassAlias' cases if you stick to this convention.. It's just not the Symfony-convention, but an SSP-convention..
Again, I'm not against any convention, it's just the way we work right now @ SSP and it's been serving us well.. @thijskh @jaimeperez what are your ideas and preferences here?

@tvdijen tvdijen force-pushed the refactor-routing-rebased branch from 5bc91ac to 0c91640 Aug 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.