Skip to content
This repository has been archived by the owner on May 1, 2019. It is now read-only.

Feature/modules from user #444

Merged
merged 7 commits into from
Mar 5, 2015
Merged

Feature/modules from user #444

merged 7 commits into from
Mar 5, 2015

Conversation

gianarb
Copy link
Contributor

@gianarb gianarb commented Mar 2, 2015

Reference to #431

@gianarb
Copy link
Contributor Author

gianarb commented Mar 2, 2015

I have changed /u/:owner in /user/:owner can you help me with a review?! 👍
@ins0 @Ocramius @localheinz

'options' => [
'route' => '/user/:owner',
'constrains' => [
'owner' => '[a-zA-Z][a-zA-Z0-9_-]*',
Copy link
Member

Choose a reason for hiding this comment

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

A bit restrictive IMO. Not sure what github's username limitations are

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mm I don't find a github's username restriction

Copy link
Contributor

Choose a reason for hiding this comment

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

Username may only contain alphanumeric characters or single hyphens, and cannot begin or end with a hyphen

so we would end up like this ^^ ^(?!.*--.*)([a-zA-Z0-9][a-zA-Z0-9-]+?[a-zA-Z0-9])$|(^[a-zA-Z0-9]+$) 🐠

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😕

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

without the second match group you wouldn't catch usernames with les then 3 characters 😃

@gianarb
Copy link
Contributor Author

gianarb commented Mar 3, 2015

@localheinz have you feedback for this feature? :) Thanks

@localheinz
Copy link
Member

@gianarb

Going to look into this tomorrow, ok? I've had one 🍸 too many to review now!

@localheinz localheinz self-assigned this Mar 3, 2015
'query' => $query,
]);

return $viewModel;
Copy link
Contributor

Choose a reason for hiding this comment

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

could be inlined

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 run CSFIX after all feedback :D

Copy link
Member

Choose a reason for hiding this comment

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

@gianarb

Does the CSFIX thing automatically inline the variable, as in

return new ViewModel([
    'modules' => $modules,
    'query' => $query,
]);

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

aaah ok sorry :)

@localheinz
Copy link
Member

@gianarb

Thank you for implementing the changes! Other than #444 (comment) I wouldn't want to complain about anything.

Happy to help with tests, but don't want to steal anything.

@gianarb
Copy link
Contributor Author

gianarb commented Mar 4, 2015

@localheinz @Ocramius sorry for $this 😖 today I'm very inattentive..
Fixed, I added a test and rebased

@localheinz
Copy link
Member

@gianarb @ins0 @Ocramius

Merging, we can still re-iterate, yay!

localheinz added a commit that referenced this pull request Mar 5, 2015
@localheinz localheinz merged commit 0e3a204 into zendframework:master Mar 5, 2015
@localheinz localheinz mentioned this pull request Mar 5, 2015
1 task
@gianarb
Copy link
Contributor Author

gianarb commented Mar 5, 2015

I'm in war with " :P Thanks

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants