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

Break the routes #38

Merged
merged 5 commits into from
Jan 2, 2018
Merged

Break the routes #38

merged 5 commits into from
Jan 2, 2018

Conversation

devvoh
Copy link
Owner

@devvoh devvoh commented Dec 31, 2017

Arrays are stupid. Now that there's ->get(), ->post(), etc. available on \Parable\Framework\App, it's much better to use those, especially since, as methods, they offer type hinting whereas the array method is far too loose and fragile.

@devvoh
Copy link
Owner Author

devvoh commented Jan 2, 2018

Note: this completely borks named routes (and therefore all methods that work with routes based on their name, which happen to be fantastic methods), so a further commit which fixes that is coming later today.

This will add $name = null to the parameter list for the simple routing methods before $templatePath.

Copy link
Contributor

@dmvdbrugge dmvdbrugge left a comment

Choose a reason for hiding this comment

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

Small stuff comments.

Question though: if this breaks named routes, how come test suite passes? 😜

CHANGELOG.md Outdated
@@ -67,6 +67,7 @@ __Backwards-incompatible Changes__
- `\Parable\Routing\Route` has lost the ability to use typed params. Too much code for too little gain. If you need typed parameters, I suggest you figure something out for yourself.
- `\Parable\Routing\Route` no longer supports `template` for the template path, but the more correctly named `templatePath` instead. Because of this, it now checks more strictly whether valid properties are set through the Routing array. `setDataFromArray()` attempts to call setters named like the properties. Any that are not available with a setter will throw an Exception. All properties are now also `protected`.
- `\Parable\Routing\Router` now also supports adding a completely set-up `Route` object directly (or in an array), without having to pass them as arrays, through `addRoute()` and `addRoutes()`. These methods already existed, but those are now renamed to `addRouteFromArray()` and `addRoutesFromArray()`.
- The default structure's `\Config\App` is now expected to extend the abstract base class `\Parable\Framework\Routing\AbstractRouting` and the old `Interfaces\Routing` interface has been removed. **This removes passing routes as arrays!**. See the new implementation, using the method calls on `\Parable\Framework\App` in the structure's `app/Routing/App.php_struct`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this line start with the following?

The default structure's \Routing\App

(As seen in the struct file)

/*
* It's also possible to specify multiple methods to respond to. Also, callables.
*/
$this->app->multiple(["GET", "POST"], "/callable", function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use Request::METHOD_*?

],
];
/*
* Valid methods are GET/POST/PUT/PATCH/DELETE/OPTIONS. If a url matches but the method doesn't,
Copy link
Contributor

Choose a reason for hiding this comment

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

Reference Request::METHOD_* or Request::VALID_METHODS constants?

@devvoh
Copy link
Owner Author

devvoh commented Jan 2, 2018

@dmvdbrugge The tests still worked because it wasn't actively under test, but passively. That's why I've added some tests to check for both named routes and unnamed (which will generate a uniqid for it instead).

@devvoh devvoh merged commit 5372a5b into develop Jan 2, 2018
@devvoh devvoh deleted the break-the-routes branch February 5, 2018 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants