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

API Refactor Form request handling into FormRequestHandler #6692

Merged
merged 1 commit into from
Mar 10, 2017

Conversation

tractorcow
Copy link
Contributor

@tractorcow tractorcow commented Mar 9, 2017

@dhensby
Copy link
Contributor

dhensby commented Mar 9, 2017

There are some CS failures

if (!$pageURL) {
return null;
}
if (Director::is_site_url($pageURL)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why should the getReferer be opinionated about whether the url is a site url or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I can call it getInternalReferer, but the intent of this method is to get only a referer that's safe to redirect to.

I could add a flag to it to allow external urls to be allowed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/**
 * Get referrer to redirect back to and safely validates it
 */

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll split it into getReferer() (raw) and getReturnReferer() (safely validates)

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe getSafeReferer is all we need as the code to get the referer header is a one liner anyway?

@@ -2,17 +2,15 @@

namespace SilverStripe\Security;

use Page;
Copy link
Contributor

Choose a reason for hiding this comment

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

Page is never used in this class (apart from for a phpdoc type hint)

Copy link
Contributor Author

@tractorcow tractorcow Mar 9, 2017

Choose a reason for hiding this comment

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

I'll change this string reference to a class constant reference.
private static $page_class = Page::class;

@tractorcow
Copy link
Contributor Author

Fixed up linting issues, split getReferer() into getReferer() and getReturnReferer(), and replaced a string literal with class constant in Security.php.

@dhensby
Copy link
Contributor

dhensby commented Mar 10, 2017

The CMS PR needs fixing before we can merge

API Add HasRequestHandler interface
API Refactor Link() and url handling behaviour from Controller into RequestHandler
API RequestHandler classes now must define url_segment to have a default Link()
API Clean up redirectBack()
@tractorcow
Copy link
Contributor Author

Fixed @dhensby

@dhensby dhensby merged commit bba86bb into silverstripe:master Mar 10, 2017
@tractorcow tractorcow deleted the pulls/4.0/form-refactor branch March 12, 2017 21: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