Skip to content

Adds Route support in App->route()#239

Closed
emersion wants to merge 1 commit into
ratchetphp:masterfrom
emersion:master
Closed

Adds Route support in App->route()#239
emersion wants to merge 1 commit into
ratchetphp:masterfrom
emersion:master

Conversation

@emersion

@emersion emersion commented Oct 6, 2014

Copy link
Copy Markdown

This PR allows to provide a Route object as $path in order to be able to build more complex endpoints, such as:

$app->route(new Route('/{any}', array(), array('any' => '.*')), $httpServer);

(this redirects all URLs to the controller)

@cboden

cboden commented Nov 8, 2014

Copy link
Copy Markdown
Member

I'm on the fence about this approach. On the plus side it maintains some ideal configuration for the server security. On the minus side it's breaking both the App and Route abstractions.

The RouteCollection has been exposed as a public property $routes on the App class. Is that open enough to fit your more complex endpoints?

@emersion

Copy link
Copy Markdown
Author

The problem is that we have to set properties such as _controller, Origin and call setHost manually. That's not very convenient, do you have another idea?

@saidmoya12

Copy link
Copy Markdown

For security reasons you cannot implement this directy on the bundle but..

1 you can override the class, (extends)

  1. You can install a bundle or library for cors domain

You don't must implement that directly on a bundle becouse that is an specific situation

@mikegioia

Copy link
Copy Markdown

Has there been any update on this? I think the only difference between @emersion 's pull request and the suggestion from @cboden is that the PR sort of collects what I'd have to do anyway with the public $routes collection.

The PR is more of a convenience method in accepting a Route object instead of a path string imo. However, what's painful about manually hitting $routes is that I have to emulate all of the decorations Ratchet adds when creating $decorated. I'll end up copying out more code than if I just extended Ratchet\App with my own class :(

@sudoanand

sudoanand commented Oct 9, 2018

Copy link
Copy Markdown

This is a real requirement and would love to see this feature.

Please have a look at this related PR: #689

@clue clue deleted the branch ratchetphp:master May 14, 2025 11:12
@clue clue closed this May 14, 2025
@clue

clue commented May 15, 2025

Copy link
Copy Markdown
Member

Looks like this PR was automatically closed when we renamed our master branch to 0.4.x (see #1091) because the fork was already deleted.

If you think this is still relevant, please update your branch to use 0.4.x as the base and open a new PR. Thanks for your contribution, and sorry for the hassle!

@emersion

Copy link
Copy Markdown
Author

I have no plans to continue working on this. If someone else wants to pick this up, please feel free!

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.

7 participants