8000 Add the Countable interface to RouteCollection. by Crell · Pull Request #4642 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

Add the Countable interface to RouteCollection. #4642

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

Merged
merged 1 commit into from
Jun 25, 2012
Merged

Add the Countable interface to RouteCollection. #4642

merged 1 commit into from
Jun 25, 2012

Conversation

Crell
Copy link
Contributor
@Crell Crell commented Jun 23, 2012

Since RouteCollection is a fancy-pants array of Routes, and it already is iterable, it would be nice if it were also countable.

There may be optimizations to be had here, but I figure this is a decent start. There should be no BC breaks here, just some DX convenience.

*/
public function count()
{
return count($this->routes->all());
Copy link
Member

Choose a reason for hiding this comment

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

wrong indentation here. It should use 4 spaces

@Crell
Copy link
Contributor Author
Crell commented Jun 23, 2012

You Symfony people and your gratuitous whitespace... :-P

Both fixed and rebased.

@travisbot
Copy link

This pull request fails (merged 6b588eb3 into 0d4b02e).

@travisbot
Copy link

This pull request fails (merged abd74cf5 into 0d4b02e).

@travisbot
Copy link

This pull request fails (merged c350944 into 0d4b02e).

@mvrhov
Copy link
mvrhov commented Jun 24, 2012

@Crell running php-cs-fixer on your changes will prevent you from being stoffed :P about CS

@stof
Copy link
Member
stof commented Jun 24, 2012

@mvrhov depends. The CS fixer will replace tabs with 4 spaces, but it will not fix it if using 2 spaces instead of 4. it cannot fix everything

@vicb
Copy link
Contributor
vicb commented Jun 24, 2012

@Crell what is your use case ?

@fabpot
Copy link
Member
fabpot commented Jun 25, 2012

I'm not opposed to the change but what's the use case?

@lsmith77
Copy link
Contributor

This is semi related to ideas we have for the dynamic router in the CMF. The 2 main reasons for the existence of the dynamic router are:

  1. the fact that end users should be able to define new routes
  2. the fact that there might just be too many routes to dump those efficiently to PHP

Now in some cases despite 1) the users might still want to dump key routes or even all routes as part of some deployment process to mod_rewrite or PHP when moving content from a staging database to a production database for better performance and reduced load on the database server.

Also even with the dynamic router it would be good to have tools available like app/console router:debug.

But for both the use case of dumping some/all routes or when listing routes it might be necessary to implement some stop gap solutions to first check if this wouldn't lead to a too big collection. So it could be useful to be able to use whatever efficient solution the dynamic data store has for determining the count before starting to actually read the data from the data source.

fabpot added a commit that referenced this pull request Jun 25, 2012
Commits
-------

c350944 Add the Countable interface to RouteCollection.

Discussion
----------

Add the Countable interface to RouteCollection.

Since RouteCollection is a fancy-pants array of Routes, and it already is iterable, it would be nice if it were also countable.

There may be optimizations to be had here, but I figure this is a decent start.  There should be no BC breaks here, just some DX convenience.

---------------------------------------------------------------------------

by Crell at 2012-06-23T17:01:43Z

You Symfony people and your gratuitous whitespace... :-P

Both fixed and rebased.

---------------------------------------------------------------------------

by travisbot at 2012-06-23T17:25:31Z

This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1689898) (merged 6b588eb3 into 0d4b02e).

---------------------------------------------------------------------------

by travisbot at 2012-06-23T17:26:06Z

This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1689902) (merged abd74cf5 into 0d4b02e).

---------------------------------------------------------------------------

by travisbot at 2012-06-23T18:10:57Z

This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1689975) (merged c350944 into 0d4b02e).

---------------------------------------------------------------------------

by mvrhov at 2012-06-24T07:11:12Z

@Crell running [php-cs-fixer](http://cs.sensiolabs.org/) on your changes will prevent you from being stoffed :P about CS

---------------------------------------------------------------------------

by stof at 2012-06-24T09:34:24Z

@mvrhov depends. The CS fixer will replace tabs with 4 spaces, but it will not fix it if using 2 spaces instead of 4. it cannot fix everything

---------------------------------------------------------------------------

by vicb at 2012-06-24T17:14:14Z

@Crell what is your use case ?

---------------------------------------------------------------------------

by fabpot at 2012-06-25T06:59:00Z

I'm not opposed to the change but what's the use case?

---------------------------------------------------------------------------

by lsmith77 at 2012-06-25T11:22:01Z

This is semi related to ideas we have for the dynamic router in the CMF. The 2 main reasons for the existence of the dynamic router are:
1) the fact that end users should be able to define new routes
2) the fact that there might just be too many routes to dump those efficiently to PHP

Now in some cases despite 1) the users might still want to dump key routes or even all routes as part of some deployment process to mod_rewrite or PHP when moving content from a staging database to a production database for better performance and reduced load on the database server.

Also even with the dynamic router it would be good to have tools available like ``app/console router:debug``.

But for both the use case of dumping some/all routes or when listing routes it might be necessary to implement some stop gap solutions to first check if this wouldn't lead to a too big collection. So it could be useful to be able to use whatever efficient solution the dynamic data store has for determining the count before starting to actually read the data from the data source.
@fabpot fabpot merged commit c350944 into symfony:master Jun 25, 2012
@Crell
Copy link
Contributor Author
Crell commented Jun 25, 2012

Not the use case I'd been looking at, but still a valid one.

Although, process question. If Travis just said above that this PR failed tests (although I'm unclear why), why was it merged anyway? (I'm used to Drupal where if the testbot doesn't like it, it doesn't happen. Period.)

@stof
Copy link
Member
stof commented Jun 25, 2012

@Crell because @fabpot fixed the code himself before pushing the merge

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.

7 participants
0