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

mvc - larger overhaul replacing Phalcon Application #6389 #7455

Merged
merged 1 commit into from May 13, 2024

Conversation

AdSchellevis
Copy link
Member

(#6389)

This removes most phalcon code currently being used on our end, except the Volt templates (which are re-wrapped) and a translation class we can easily replace later in a separate commit. Consumers of our controller classes shouldn't notice a difference as the used objects and methods are named the same.

The most notable changes are the following ones:

  • Exceptions about not being able to find a requested path now break down into different exceptions inheriting from DispatchException, which makes it easier from the entrypoint (api.php, index.php) to catch and handle accordingly.
  • When not in development mode, raw exceptions are not being returned anymore, which increases security
  • The Dispatcher class is reponsible for object construction and mapping validation (valid uri, but no object found)
  • The Router class replaces previous Application class, it disects offered uri's into namespaces, classnames and methods to call.

In the long run there should be a seperate controller for controllers using volt templates or api calls, but as the existing ones don't distinct between this and the output handling is different now, we can park this for a later moment in time (the performance penalty should be rather low).

Some unused functionality has been removed, for example support for the X-HTTP-Method-Override header in Request->getMethod() (see https://github.com/phalcon/cphalcon/blob/44243c07658d060cd8a21761743b0f4fc01641aa/phalcon/Http/Request.zep#L599-L609).

@AdSchellevis AdSchellevis added the cleanup Low impact changes label May 12, 2024
@AdSchellevis AdSchellevis self-assigned this May 12, 2024
src/opnsense/www/api.php Outdated Show resolved Hide resolved
src/opnsense/www/api.php Outdated Show resolved Hide resolved
Copy link
Member

@fichtner fichtner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it very much, great work! left some notes for future changes perhaps.

@fichtner fichtner linked an issue May 13, 2024 that may be closed by this pull request
2 tasks
This removes most phalcon code currently being used on our end, except the Volt templates (which are re-wrapped) and a translation class we can easily replace later in a separate commit.
Consumers of our controller classes shouldn't notice a difference as the used objects and methods are named the same.

The most notable changes are the following ones:

* Exceptions about not being able to find a requested path now break down into different exceptions inheriting from DispatchException, which makes it easier from the entrypoint (api.php, index.php) to catch and handle accordingly.
* When not in development mode, raw exceptions are not being returned anymore, which increases security
* The Dispatcher class is reponsible for object construction and mapping validation (valid uri, but no object found)
* The Router class replaces previous Application class, it disects offered uri's into namespaces, classnames and methods to call.

In the long run there should be a seperate controller for controllers using volt templates or api calls, but as the existing ones don't distinct between this and the output handling is different now, we can park this for a later moment in time (the performance penalty should be rather low).

Some unused functionality has been removed, for example support for the  X-HTTP-Method-Override header in Request->getMethod() (see https://github.com/phalcon/cphalcon/blob/44243c07658d060cd8a21761743b0f4fc01641aa/phalcon/Http/Request.zep#L599-L609).
@fichtner
Copy link
Member

Merged, thanks! 🥳

@fichtner fichtner deleted the new_mvc_routing branch May 13, 2024 08:38
@kriansa
Copy link

kriansa commented May 23, 2024

@fichtner I understand this is probably not the best place to ask this, so I apologize upfront -- but why is Phalcon being removed? I'm not really aware of what's happening in PHP community but I'm curious to know why it's being considered frowned upon.

@AdSchellevis
Copy link
Member Author

@kriansa lowering the dependency chain and ease of maintenance in the long run. For us Phalcon doesn't add much value and over the years we spend quite some time handling quirks which would have been easier to solve without the framework in between. We will keep some of it (the volt templates), due to lack of choices in that area.

@fichtner
Copy link
Member

@kriansa fair question, don't worry. I think at some point we might even break out volt into its own module so we are not bogged down waiting for Phalcon to support a newer PHP version in the long-term https://github.com/phalcon/volt

@kriansa
Copy link

kriansa commented May 23, 2024

Thanks guys, I appreciate the transparency 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Low impact changes
Development

Successfully merging this pull request may close these issues.

mvc: Phalcon framework dependency
3 participants