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

Discussion: shaarli controllers #631

Closed
wants to merge 1 commit into from

Conversation

ArthurHoaro
Copy link
Member

Disclaimer: this PR is only here for discussion.

Our main problem with Shaarli's codebase from the beginning, is that almost everything is contained in index.php. We worked a lot to put specific code in their own classes/functions in /application.

Now, I think it's time to go further and split index.php code into specific classes designed for any request. This means:

  • tear down renderPage() which is about 1k line of code
  • refactor the Router class which doesn't handle every action, for now
  • be able to unit test a lot more of thing (e.g. here, request and conf on the login pages).

This PR is an example for the page login.html. I'm willing to work on this for the rest of Shaarli, but I'd like your opinion on the implementation and technical choices before going further.

Basically:

  • I added a Controller abstract class, which is kind of container, handling all of our global classes (configuration, page rendering, plugins, request parameters, etc.).
  • In index.php, a specific Controller is called for any action, based on the Router page definition.
  • Every controller extends the main abstract class, and will have to call:
    • First redirect(): if a header redirection is needed.
    • Then render() which will render the page through the template engine.

@ArthurHoaro ArthurHoaro self-assigned this Aug 9, 2016
@virtualtam
Copy link
Member

We definitely need such a View/Controller system to tidy up the codebase. I'm not too familiar with how MVC-like patterns are implemented in PHP, as I've mostly been using Python libs & frameworks for Web-related musings, and they have a slightly different approach when it comes to how applications are designed (MVC with some twists).

Here is a possible component repartition:

  • models: how data is internally stored and accessed - that would mostly be LinkDB (or its offsprings)
  • views (I'm biased towards Python libs here):
    • each view corresponds to a given action/feature, e.g. "add a link", "edit a link"
    • it comprises the preparation of the internal data (model queries, additional processing) and potentially a call to render machine-readable code (HTML, JSON, etc.), using a template engine
  • routes: they map HTTP queries to given views

This would amount to:

  • having a view per page/action
  • letting views fetch data from LinkDB et al., then pass processed data as a context to a template engine for rendering
  • mapping Router entries to the corresponding views

If I get things right, the proposed Controller would encapsulate view operations, so they can benefit from a global context through ConfigManager, and receive additional data and template processing from plugin hooks?

@ArthurHoaro
Copy link
Member Author

I'm familiar with Symfony in PHP, and Spring and Struts in Java. All of them don't let you worry about what's done in index.php, but we have to.

models

Frameworks usually use an ORM, but we only have LinkDB for now. It needs some work, #351 #445, but it's not related to this.

views/controller

I started by calling it views, but they do more that just rendering the display. In Symfony, views are templates, and in my understanding of MVC, view is the part where the page is displayed. Here, it handles the user request and acts depending on it. It also call the template engine rendering, though.

Otherwise, yes, your 2 bullet points are right.

routes

Not sure how they're handled in frameworks, but here, the router only has to give which view/controller should be called, based on the request (post/get request todo). HTTP info will be passed anyway.

having a view per page/action

Yes

letting views fetch data from LinkDB et al., then pass processed data as a context to a template engine for rendering

Hmm each view/controller will have access to their main class objects, including LinkDB, but also the config manager, the plugin manger, etc.. Then all necessary data will be passed to the template, as it is done in the current index.php.

mapping Router entries to the corresponding views

Yes, we need an improvement on this, since all pages (or actions) aren't yet handled by the router.

If I get things right, the proposed Controller would encapsulate view operations, so they can benefit from a global context through ConfigManager, and receive additional data and template processing from plugin hooks?

ConfigManager, and other objects, so they'll basically handle everything done in index.php in their own objects.

@ArthurHoaro
Copy link
Member Author

I have something more or less working, and I believe it's looking good. It also made me realize that we abuse of redirections, while we could just call the appropriate templates. Working on this in the future would be easier, and could have good impact on performances.

Anyway, it's going to take me forever to unit test it, but I'll go for it.

@kalvn
Copy link

kalvn commented Oct 3, 2016

Indeed the MVC as used in Symfony (or at least a simplified version of it) seems to be the way to go.

If you are looking for inspiration, there are some popular micro-frameworks usually presented as routers/controllers. Their primary role is actually to map a route to a function (as you described above) and then you do what you want in it (rendering a template, reading or writing DB, etc.).

The Slim Framework is one of them (that I particularly like). The documentation is quite short to read and could be useful.

@ArthurHoaro
Copy link
Member Author

Closing - see #655

@ArthurHoaro ArthurHoaro deleted the controllers branch October 20, 2016 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants