zeruniverse / Password-Manager

An online keepass-like tool to manage password. client-side AES encryption!
Other
170 stars 44 forks source link

[Enhancement] Move to a MVC architecture #235

Closed schams-net closed 4 years ago

schams-net commented 5 years ago

Problem

At the moment, only parts of the application separate business logic from the "view" for example.

Suggested Solution

A strict and clean separation between model, view and controller (MVC) would make ongoing maintenance and possible extensions easier in the future.

Files belonging to the view should be HTML only and should not include any PHP code (maybe make use of a simple template engine). Files belonging to the business logic (controller and model) should/could be stored separately (e.g. in a folder Classes/) and should not contain any HTML code.

Changes Required

Additional Notes

Modern template engines in the PHP world today:

BenjaminHae commented 5 years ago

I'd support a complete rewrite of the backend (including #232, #233). But this should focus more on providing the backend as a webservice. So a templating engine is not needed.

That way multiple device types can be easily supported. Currently the "html" files only use php for two things:

The first part could be done in the webservers configuration (.htaccess or similar) but that increases the effort necessary to install the password manager. The second part can be solved by using angular.

BenjaminHae commented 5 years ago

I currently favor symfony (maybe Silex) and swagger, but as our php code is very easy and reable I'm afraid using a framework might add unnecessary complexity. I'm currently working on a rewrite that is more modular: https://github.com/BenjaminHae/Password-Manager/tree/backendPluginSystem That also implements #239.

schams-net commented 5 years ago

That makes sense. Your note "our php code is very easy and readable [...]" and the fact that most of the logic happens on the client side anyway are convincing arguments against using a sophisticated template engine. This would be overkilled.

Would you accept a PR that implements a very basic, simple solution to separate the view from any PHP files? Given basic.php contains functions that deal with the view (HTTP headers, page header and page footer) only, I reworked this component and moved the logic to a class. At the same time, I separated all HTML code into .html files. Main benefits:

See commit da3d41218e0157a5fa3cff26c8f86e6cb0bf5026 (browse files)

If this looks ok for you, I am happy to create a PR. However, it would be great, if you could merge PR 238 first to avoid merge conflicts.

By the way: I see these changes independent from your work on "backendPluginSystem". This sounds like a much bigger piece of work and I don't have a problem, if my update(s) get removed when a new architecture is in place :-)

BenjaminHae commented 5 years ago

Why not completely remove php from the view? Maybe we could just offer plain html files without any templating engine at runtime. Headers could be set using the .htaccess file in apache (similar settings exist for most popular webservers).

That way all files are exactly where a naive user expects them and no abstraction layer that has no real functionality is necessary.

BenjaminHae commented 5 years ago

Do you have experience concerning swagger and symfony? I'm currently trying to get symfony working with the files generated by swagger-codegen but I'm not really successful.

zeruniverse commented 4 years ago

Close due to inactivity. For new features, please submit MR to the new project. #248