Commit Graph

9418 Commits

Author SHA1 Message Date
Drak
ddca4bbdac Update CHANGELOG-2.1.md 2012-04-20 18:07:41 +05:45
Fabien Potencier
736886688f merged branch vicb/finder/regex (PR #4028)
Commits
-------

f728463 [Finder] Fixes in the iterators

Discussion
----------

[Finder] Fixes in the iterators

fix: #4023
ref: #4011

- Fix regex detection
- call `file_get_contents()` only once (vs once per pattern)

[![Build Status](https://secure.travis-ci.org/vicb/symfony.png?branch=finder/regex)](http://travis-ci.org/vicb/symfony)

@gajdaw thoughts ?
2012-04-20 14:18:46 +02:00
Victor Berchet
3937561b20 [FrameworkBundle] Look for translations in %kernel.root_dir%/Resources/%bundle%/translations (fix #4018) 2012-04-20 14:17:18 +02:00
Victor Berchet
f72846305e [Finder] Fixes in the iterators 2012-04-20 12:38:15 +02:00
Fabien Potencier
539634cbaa merged 2.0 2012-04-20 12:18:51 +02:00
Fabien Potencier
5f9817167e merged branch drak/genericevent (PR #3784)
Commits
-------

70a6de9 [EventDispatcher] Remove data property
4ff92b9 [EventDispatcher] Added generic event class.

Discussion
----------

[EventDispatcher] Added generic event class.

Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -

Provides a ready to go all purpose event object.

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

by Koc at 2012-04-04T17:53:41Z

What use case of this?

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

by drak at 2012-04-04T17:57:46Z

It generically fits observer pattern use for those who do not want to make a specific event class for each and every little thing.  This is akin to EventArgs + a subject.

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

by fabpot at 2012-04-20T07:55:05Z

This is basically what we had in symfony 1.x.

Some comments:

 * I would remove the `data` as it makes things more complex and more difficult to understand (as arguments is an array, you can put your data there too);
 * I would use the standard `set`, `get`, `all`, ... methods for the `arguments` related methods

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

by drak at 2012-04-20T08:11:31Z

OK, I'll rework the class.  Thank you for your consideration.

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

by drak at 2012-04-20T08:56:53Z

@fabpot - I did some work on the class, but it seemed strange to not use get/setArgument because there is another property, the subject. This is mentioned in the conventions guide:

```
For many relations where the convention does not apply, the following methods must be used instead (where XXX is the name of the related thing):
```

If you still want me to change it, let me know.
2012-04-20 11:33:59 +02:00
Drak
70a6de9896 [EventDispatcher] Remove data property 2012-04-20 14:49:32 +05:45
Drak
4ff92b93ce [EventDispatcher] Added generic event class. 2012-04-20 14:49:22 +05:45
Fabien Potencier
f8407dd2cc merged branch willdurand/propel-autoload (PR #4025)
Commits
-------

496878c [Propel1] Removed useless require in autoload.php.dist

Discussion
----------

[Propel1] Removed useless require in autoload.php.dist

Since we have Composer, and a classmap, no need to trigger the Propel's autoloader
2012-04-20 10:00:20 +02:00
William DURAND
496878cf2d [Propel1] Removed useless require in autoload.php.dist
Since we have Composer, and a classmap, no need to trigger the Propel's autoloader
2012-04-20 09:46:34 +02:00
Fabien Potencier
9d22eb6007 merged branch willdurand/propel-transformer (PR #4024)
Commits
-------

9447b84 [Propel1] Allowed PropelObjectCollection only in CollectionToArrayTransformer
e43d0fa reverse transform into PropelObjectCollection

Discussion
----------

Propel transformer

Prefer this PR over #3966
2012-04-20 09:39:45 +02:00
William DURAND
9447b84acb [Propel1] Allowed PropelObjectCollection only in CollectionToArrayTransformer 2012-04-20 09:32:01 +02:00
Toni Uebernickel
e43d0fa0fd reverse transform into PropelObjectCollection 2012-04-20 09:28:12 +02:00
Fabien Potencier
56a4ab7903 made some private protected as many users needs to override the default implementation anyway (closes #3942) 2012-04-20 09:20:31 +02:00
Fabien Potencier
2786f21822 fixed CS 2012-04-20 09:05:48 +02:00
Fabien Potencier
74cc38004b merged branch gajdaw/finder_search_by_contents (PR #4011)
Commits
-------

218813c [Finder] contains(), notContains()
33e119a Merge branch 'master' of https://github.com/symfony/symfony into finder_search_by_contents
082d86e [Finder] content(), notContent() methods

Discussion
----------

[Finder] search by contents

Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -

Sometimes I need to search for files containing some text:

```
$finder
    ->content('lorem')->notContent('ipsum')
    ->content('/^Begining/m')->notContent('/the end$/m');
```

I don't know how to tests exceptions thrown by `file_get_contents()` calls.

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

by gajdaw at 2012-04-19T15:53:05Z

To keep it as close as possible to `name` and `notName`.
2012-04-20 09:04:33 +02:00
Fabien Potencier
3e2b39dd21 merged branch michal-pipa/symlink-fix (PR #4012)
Commits
-------

94bee7a [Filesystem] symlink() creates target directories

Discussion
----------

[Filesystem] symlink() creates target directories

Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes [![Build Status](https://secure.travis-ci.org/michal-pipa/symfony.png?branch=symlink-fix)](http://travis-ci.org/michal-pipa/symfony)
Fixes the following tickets: #3967
Todo: -

Changed symlink() method behavior to recursively create target directory if it does not exist. It makes Filesystem component methods more consistent since copy() does the same. It is also more convenient.

Also mirror() fails to create symlink in non-existent directory (if we don't want to change symlink(), than we need to fix mirror()).

Fixes: #3967
2012-04-20 08:50:33 +02:00
Fabien Potencier
e1c8a2fdbd [WebProfilerBundle] made some small changes to the WDT 2012-04-20 08:44:34 +02:00
Fabien Potencier
f7287db9be [WebProfilerBundle] deprecated the verbose option for the WDT as this is not needed anymore (responsive design FTW) 2012-04-20 08:22:09 +02:00
Fabien Potencier
e2326a7b73 merged branch shiroyuki/master (PR #3850)
Commits
-------

75c7d3a Fixed the link to the method with onclick event.
ecbabec renamed 'Request handler' to 'Controller' and 'Route ID' to 'Route name'.
3c5ede4 Add a new query to display all information.
f6a866b Merged CSS for the toolbar in both embedded mode (on each page) and profiler.
306533b Updated the responsive design in addition to the scenario with authenticated users and exception notification.
4a3312b Updated the toolbar with the responsive design (normal-to-large scenario).
1eec2a2 Updated the toolbar with the responsive design (normal-to-small scenario).
03c8213 Refactored the CSS code for the toolbar out of the template.
37843b3 Updated with PHP logo (only the text).
d5e0ccc Made the toolbar to show the version, memory usage, the state of security (both a abbreviation and an associate description) and number of DB requests and request time.
37ad8a6 Removed the check for verbose and adjusted the style when the toolbar is on the top of the page.
67b0532 Redesigned the WDT.

Discussion
----------

Re-design the debugging toolbar

The toolbar is very useful and containing lots of information. However, as there are too much information, it is very distracting and the toolbar area somehow ends up taking too much space and then becomes something like a panel.

The main purpose of this pull request is to hide any information and show only whenever the user wants to see, except the status code and response time.

This is based on [the pull request #3833](https://github.com/symfony/symfony/pull/3833) with the feedbacks and for 2.1 (master).

The testing app is available at http://home.shiroyuki.com.

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

by stof at 2012-04-10T06:24:36Z

@shiroyuki your testing app denies the access because of the restriction in app_dev.php

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

by shiroyuki at 2012-04-10T06:27:27Z

@stof: I'm sorry. It should be working now.

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

by stof at 2012-04-10T06:45:39Z

Moving the toolbar to the top of the page means it will hide some content of the page. You should keep it at the bottom

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

by shiroyuki at 2012-04-10T06:48:28Z

Just a moment ago, I changed the position of the toolbar via `config_dev` so I could check when WDT is on the top.

I just reverted the config file. :D

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

by fabpot at 2012-04-10T06:55:16Z

Some comments:

 * I would have kept the number of database request as this number is probably the one everybody should have a look at on every page.
 * I would have used the original PHP logo (in black and white) instead of a non-standard one

But overall, this is a very nice improvement.

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

by stloyd at 2012-04-10T06:55:43Z

There is an issue with "bubbling" at Firefox 11 (at least), when you hover `<a>` element, the hover event seems to be "launched" twice.

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

by fabpot at 2012-04-10T06:56:13Z

As the verbose mode has been removed from the template, it should also be removed from the configuration (I can do that after merging if you don't know how to do that).

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

by shiroyuki at 2012-04-10T07:05:31Z

@stloyd I noticed that too. As I couldn't find the same issue on Webkit-based browsers and all effects on this toolbar heavily relies on CSS, it could have been a glitch on Firefox.

@fabpot I'll see what I can do with the number of DB request and the logo.

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

by asm89 at 2012-04-10T07:26:28Z

Will there be options to somehow keep the debug toolbar 'expanded' or something? I guess the folding of the sf and php information makes sense, but I personally look at the request/time/memory/security and query parts of the toolbar a lot. As my browser window is big enough to show all information at once, this would be a huge step backwards imo.

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

by XWB at 2012-04-10T07:28:38Z

Agreed with @asm89, I also want the option to show all the information on my screen.

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

by fabpot at 2012-04-10T08:28:00Z

I tend to agree too with @asm89. What about reusing the `verbose` option for that. This was already its purpose anyway.

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

by shiroyuki at 2012-04-10T14:56:45Z

How about using media query?

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

by shiroyuki at 2012-04-11T02:20:32Z

Please note that the latest commit still doesn't have the new logo for PHP.

As DoctrineBundle now has its own repository, the change to show the number of DB requests is already done via DoctrineBundle's [PR 57](https://github.com/doctrine/DoctrineBundle/pull/57).

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

by guilhermeblanco at 2012-04-11T02:50:47Z

@fabpot @shiroyuki as soon as this patch is merged I will do the same on DoctrineBundle.
All you need to do is look at me over our desks' separator. =D

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

by shiroyuki at 2012-04-11T03:17:41Z

The last commit has the updated PHP logo. Unfortunately as @stloyd and @guilhermeblanco pointed out, the flicking on the toolbar when the mouse is over might have been due to the CSS issue on Firefox.

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

by Tobion at 2012-04-11T04:46:36Z

Nice work shiroyuki. I always had the feeling the toolbar can be improved. Good that you got this one going.
I would remove the verbose option (rarely nobody changes it) and use media queries to accomplish a responsive design that shows as much information as possible. And only shows the most important facts when there is not enough space.
E.g. the symfony version could be removed if it doesn't fit on the screen because it's mostly static from request to request.

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

by Tobion at 2012-04-11T04:48:45Z

Another idea: Add a panel "PHP Info" to the profiler that shows the output of `phpinfo()`. This panel is linked from the PHP logo in the WDT which currently has no link on it.

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

by shiroyuki at 2012-04-11T15:47:51Z

@Tobion: It would be an overkill if `phpinfo()` was visible in the toolbar. Additionally, the toolbar doesn't fit to show that amount of information. Plus, the information released by `phpinfo()` is also static and easily obtained by a simple PHP script. I don't think that WDT should be showing this information.

Please note that the media query is not yet implement. The followings are still unknown to me:

* should we support the toolbar for mobile device?
* what is the minimum screen size?

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

by Tobion at 2012-04-11T15:52:43Z

@shiroyuki you misunderstood me. phpinfo() should be a new panel in the PROFILER, not the WDT. It is reachable from the WDT by clicking on the PHP logo. But that can be implemented in a seperate PR. It's just an idea and before I would implement it, I'd like to receive feedback if it would be accepted at all.

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

by fabpot at 2012-04-11T16:38:44Z

Displaying `phpinfo()` data is not in the scope of this PR.

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

by Tobion at 2012-04-11T16:48:50Z

@fabpot yeah. But would you accept such a PR or do you think it's not useful?

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

by fabpot at 2012-04-11T16:57:49Z

@Tobion The web profiler is mainly about information for the current request; so I'm not sure it would be useful to have such a tab in the profiler.

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

by vicb at 2012-04-11T17:06:15Z

@fabpot @Tobion what about adding it in the config panel ? Not sure if it is very useful but I have seen to many `phpinfo.php` in the web root folder. (It could be an expandable panel loaded via ajax like what is used for the Doctrine explain panel).

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

by shiroyuki at 2012-04-12T03:11:40Z

@tobian @vicb: what kind of information are you looking from `phpinfo()`?

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

by Felds at 2012-04-12T03:30:02Z

The equivalent for `phpinfo()` was extremely convenient and helped a lot in Symfony 1. It's out of scope but an optional panel could be nice.
Ini flags are of great help when debugging on a hurry.

👍 for that!

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

by Tobion at 2012-04-12T03:37:52Z

@shiroyuki I don't understand your question. Everything of it should be displayed. But don't worry about phpinfo(), I'll work on that in a seperate PR. You can focus on the responsive design. ;)

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

by vicb at 2012-04-12T06:54:35Z

@shiroyuki I am not looking for anything specific. Just saying I have seen many times customer code using a publicly accessible file to return the info and it would help to get ride of this file.

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

by sstok at 2012-04-12T07:59:18Z

```
should we support the toolbar for mobile device?
```

Good question, I don't think so because the screen-size is to small to show anything useful.
Maybe a small icon to display the information as overlay, including the token so you can refer to that on a bigger screen?.

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

by johnnypeck at 2012-04-13T06:45:43Z

If your interested in a useful but not so intrusive way of providing the toolbar on mobile devices perhaps take a look at what the guys at Twitter have done with the topbar navigation converting to a semi-accordion style menu on mobile in Bootstrap. I can see the usefulness. Checkout the responsive.less which makes it easy enough to include/exclude depending on screen size. I found it quite useful in a recent project.

Regarding adding a tab for phpinfo, sure it would be useful BUT if the reasoning is that some people leave a publicly available phpinfo script therefore just include it then I would not include it. There are many more useful requirements of the toolbar rather than to insulate intro to web issues. That's like saying don't include the toolbar because someone may build an application that makes the toolbar available publicly (which will happen). I've seen too many projects in my years having no clue of versioning tools that must have been built on the server, live, with filenames like indexv1.php, indexv2.php, indexTryAgain.php, db credentials in the clear, and just hoping to find a point where it works enough. And yes, I've found those scripts were publicly available and still around years after they were created; security holes and all! I'm preaching to the choir here. You'll never stop stupid. All we can do is educate by any means we have and share our knowledge with one another. Aside from that devils advocate reasoning, I would include the phpinfo tab, it does make sense in those random "did I/they compile that in" circumstances. ;-) Sorry for the rant.

+1 for mobile
sorta+1 for phpinfo
+10 for better educating on how to include anything you need so phpinfo could be a "my first foray into adding a tool to my toolbar for Symfony" tutorial in the cookbook.

Again, sorry for the long winded rant. Cheers everyone. Goodnight.

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

by shiroyuki at 2012-04-13T23:33:21Z

@stof I think we can remove the CSS.
2012-04-20 08:12:25 +02:00
Fabien Potencier
866b13e5fc merged branch Crell/flatten-exception (PR #4017)
Commits
-------

1c290d7 Add unit tests for FlattenException::getLine() and FlattenException::getFile().
a22f0cd Enhance FlattenException to include more methods from Exception.  That allows it to be used in place of Exception in more places.

Discussion
----------

[HttpKernel] Enhance FlattenException to include more methods from Exception.

I'm trying to retrofit FlattenException into Drupal, in places where Drupal expects an Exception.  That doesn't quite work though, as FlattenException only has some of the methods from Exception.  I'm not entirely clear why it only has some, but this PR adds getFile() and getLine() so that it's a more ready drop-in.  I did not add them to the toArray() method for fear of breaking BC somewhere, but that could be done as well no doubt if folks felt it was appropriate.

Note: While the parts of Drupal in question will get rewritten later anyway, I think having this information exposed is a good thing in general for logging purposes if nothing else.  It's already possible to dig it out of the trace, so this is just an improved "Developer eXperience" (DX).

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

by fabpot at 2012-04-20T04:34:54Z

I'm +1 to make `FlattenException` more "compatible" with `Exception`. Can you add the other missing methods? Also, you need to populate the `$this->file` and `$this->line` value in the constructor.

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

by Crell at 2012-04-20T04:48:40Z

I knew I was forgetting something obvious...

According to http://us.php.net/manual/en/class.exception.php, I think the only other missing method is http://us.php.net/manual/en/exception.gettraceasstring.php.  I'm not sure how useful that is, but I can try to approximate it if you think it's necessary.  (Honestly I've never used that method on an exception myself.)

I should probably add some tests, too.  Stand by for those.

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

by Crell at 2012-04-20T05:00:28Z

Now includes unit tests to make sure I didn't do anything stupid this time.  I'll hold off on getTraceAsString() for now unless you think it's needed.  (I'm not sure it is since it's harder to do and IMO less useful.)
2012-04-20 07:41:36 +02:00
Fabien Potencier
b71d42a65a tweaked previous commit 2012-04-20 07:18:49 +02:00
Roger Webb
23707bd8d6 Added Method Call, Factory Class and File Include examples. 2012-04-20 07:18:17 +02:00
Larry Garfield
1c290d7af2 Add unit tests for FlattenException::getLine() and FlattenException::getFile(). 2012-04-19 23:59:28 -05:00
Larry Garfield
a22f0cdc32 Enhance FlattenException to include more methods from Exception. That allows it to be used in place of Exception in more places. 2012-04-19 23:45:54 -05:00
Fabien Potencier
efc1a0d1b0 merged branch jalliot/patch-2 (PR #4015)
Commits
-------

7dc9361 Update autoload according to latest composer change

Discussion
----------

Update autoload according to latest composer change

[![Build Status](https://secure.travis-ci.org/jalliot/symfony.png?branch=patch-2)](http://travis-ci.org/jalliot/symfony)
2012-04-20 06:31:00 +02:00
Jordan Alliot
7dc93619d0 Update autoload according to latest composer change 2012-04-19 23:57:43 +03:00
Michał Pipa
94bee7a3d5 [Filesystem] symlink() creates target directories 2012-04-19 19:42:07 +02:00
Fabien Potencier
9b5be1df2a [Yaml] fixed tests when iconv and mbstring PHP extensions are not available 2012-04-19 19:28:53 +02:00
Włodzimierz Gajda
218813cb50 [Finder] contains(), notContains() 2012-04-19 18:19:17 +02:00
Włodzimierz Gajda
33e119a7dc Merge branch 'master' of https://github.com/symfony/symfony into finder_search_by_contents 2012-04-19 17:35:33 +02:00
Włodzimierz Gajda
082d86e3a0 [Finder] content(), notContent() methods 2012-04-19 17:34:50 +02:00
Fabien Potencier
03752564a0 merged branch vicb/umask (PR #4009)
Commits
-------

e0e451f Fix umasks in chmod() calls

Discussion
----------

Fix umasks in chmod() calls

As discussed with @Seldaek
ref: #4008
2012-04-19 16:17:16 +02:00
Victor Berchet
e0e451feb8 Fix umasks in chmod() calls 2012-04-19 15:47:04 +02:00
Fabien Potencier
4d8461f243 merged branch Seldaek/filesystemfixes (PR #4008)
Commits
-------

748bbe1 Make windows test run on windows only
e7f1295 [Filesystem] Fix Filesystem::chmod to apply umask properly
5c059aa Fix chmod() calls to apply umask
13c07d1 [Filesystem] Fix typo
c578d3a [Filesystem] Fix makePathRelative on windows with mixed paths, fix tests

Discussion
----------

[Filesystem][Others] Fix chmod method and all calls to chmod throughout the framework

Fixes the issue I mentioned in #4004 - basically php's chmod() does not apply the umask by default (unlike mkdir's mode arg which is masked by umask, from which I guess the confusion comes from).

So I expanded all cache writes and such to 0666 when they were 0644, and then mask it against umask, so that we respect the user settings a bit better.

Also fixed Filesystem::chmod which completely ignored the umask argument before.

Fixed a few tests on windows too.
2012-04-19 14:16:42 +02:00
Jordi Boggiano
748bbe17b8 Make windows test run on windows only 2012-04-19 13:51:15 +02:00
Jordi Boggiano
e7f129576e [Filesystem] Fix Filesystem::chmod to apply umask properly 2012-04-19 13:35:44 +02:00
Jordi Boggiano
5c059aa121 Fix chmod() calls to apply umask 2012-04-19 13:35:17 +02:00
Fabien Potencier
086b73506d merged branch pvanliefland/ticket_1692 (PR #3945)
Commits
-------

8bdff01 [DoctrineBridge][Form] added collection guess for array Doctrine type and array constraint type

Discussion
----------

[Form] [DoctrineBridge] Better field type guessing for array doctrine type and array validator type

Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #1692
Todo: -

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

by bschussek at 2012-04-18T08:45:17Z

Could you please add an entry to the CHANGELOG and squash your commits into one?

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

by pvanliefland at 2012-04-18T17:20:39Z

Done
2012-04-19 13:13:47 +02:00
Fabien Potencier
3c62ebf52e merged branch ruian/issue-3992 (PR #4006)
Commits
-------

45ada32 Add Support for boolean as to string into yaml extension

Discussion
----------

Add Support for boolean as to string into yaml extension

Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: 3992
Todo:
- Maybe use only boolean checker instead of YamlDumper->dump
2012-04-19 13:01:39 +02:00
Julien 'ruian' Galenski
45ada3285f Add Support for boolean as to string into yaml extension 2012-04-19 12:45:34 +02:00
Jordi Boggiano
13c07d1469 [Filesystem] Fix typo 2012-04-19 12:37:59 +02:00
Jordi Boggiano
c578d3ac57 [Filesystem] Fix makePathRelative on windows with mixed paths, fix tests 2012-04-19 12:37:15 +02:00
Fabien Potencier
811751e147 merged branch rouffj/fix-3929 (PR #4004)
Commits
-------

5fa7ec2 Fix #3929

Discussion
----------

Fix #3929
2012-04-19 12:14:24 +02:00
Joseph Rouff
5fa7ec2acb Fix #3929 2012-04-19 12:10:32 +02:00
Fabien Potencier
684e5025ad merged branch Seldaek/asseturl (PR #4002)
Commits
-------

725423c Add test to prevent regressions
3b2f542 Fix asset generation with an empty asset

Discussion
----------

[Templating] Return base URL when an empty path is given to asset()

I think it's straightforward enough in the patch. Tests pass. It's quite useful to generate the base path to send to a JS frontend.
2012-04-19 12:03:59 +02:00
Fabien Potencier
460c181951 merged branch evillemez/master (PR #3557)
Commits
-------

b7b26af [DependencyInjection] Added, implemented and tested IntrospectableContainerInterface::initialized()

Discussion
----------

[DependencyInjection] IntrospectableContainerInterface::initialized()

Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes

Added, implemented and tested `IntrospectableContainerInterface::initialized()`, which allows checking for whether or not a service id has actually been loaded, without forcing it to load.  This could be implemented in several places to prevent loading a service when it's only needed if it has been previously loaded - for example the SwiftmailBundle's event listener for the `kernel.terminate` event, which currently forces Swiftmail to load on every request to check for messages to send, even if it was not previously loaded.

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

by fabpot at 2012-03-11T09:10:32Z

Do you have any other examples in mind where it would be useful?

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

by stof at 2012-03-11T12:39:22Z

@fabpot 2 exemples:

- the Swiftmailer listener to avoid loading the initialization code of Swiftmailer in each kernel.terminate event just to check that there is no pending message in the spool (if the spool has never been retrieved, it cannot have messages) (this is the use case we discussed on IRC last night)
- closing Doctrine connections and Monolog handlers in the shutdown method without creating them if they were not used

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

by stof at 2012-03-11T12:39:53Z

However, I don't like the name of the method

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

by lsmith77 at 2012-03-11T13:26:34Z

sounds very useful

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

by evillemez at 2012-03-11T17:00:39Z

@fabpot another example:

* Forcing a session to write early, if it was previously loaded - but not having to load the session to check, thus potentially forcing a database connection (if that's how the session is being handled) when it's not needed.

@stof Would more or less verbose be better?  Like, `isServiceLoaded()` or just `loaded()`.... or a better word than "loaded" ? :)

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

by stof at 2012-03-11T17:03:11Z

@evillemez My issue is the word ``loaded``. I don't think it is clear about what it means here

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

by lsmith77 at 2012-03-11T17:04:24Z

one thing we should also keep in mind here is the scope of the service.

BTW: there are also a couple of CS violations in your PR

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

by fabpot at 2012-03-11T17:07:43Z

@stof: I agree that we should think of a better name: `initialized()` or `exists()` (to differentiate from `has()`)?

@evillemez: After choosing the name, can you work on actually using the new method for some of the use cases mentioned in this PR?

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

by lsmith77 at 2012-03-11T17:20:12Z

what i meant with "scope" was if we are only talking about services instantiated in the current scope? but i guess there is no way to handle anything else anyway.

as for name .. i think ``instantiated()`` is more fitting than ``initialized()``.
``exists()`` could be confused with ``has()`` imho

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

by stof at 2012-03-11T17:26:06Z

The current implementation only works for container-scoped services. It does not make sense  for prototyped-scope services anyway (as the container does not keep them) but supporting scoped services will be tricky IMO

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

by mvrhov at 2012-03-11T17:34:21Z

hasBeen(Used|Called|Initialized),

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

by evillemez at 2012-03-11T17:54:43Z

The next day or two I'm only around for an hour or so here and there, I may be a little slow to respond.

@stof @lsmith77 I agree with either `instantiated()` or `initialized()`, I also think `exists()` might be easily confused with `has()`

@lsmith77 Besides the opening/closing brace placements, was there anything else?

@fabpot I would happily implement it in the Swiftmail bundle - as of now that's the only area where I know absolutely it would be useful.  The Session example I mentioned was an issue I ran into working on another app in another framework, and I'm not familiar yet with the internals of the Monolog/Doctrine Bundles.  But if people could point me in the right direction I'd be willing to check it out.

I'm still relatively new to some of the Symfony2 internals, so if there are obvious things I'm missing, please don't hesitate to point them out. :)

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

by evillemez at 2012-03-11T18:00:29Z

@lsmith77 Oh... I think there were some tab issues I didn't see in my editor, I'll fix those too.

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

by stof at 2012-03-11T18:13:03Z

The places where it should be used for Doctrine and Monolog are in separate repos anyway so it cannot be part of this PR

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

by evillemez at 2012-03-12T03:38:50Z

Any thoughts on `instantiated` vs `initialized`?  I'm leaning towards `initialized`.

How should I proceed, close this request and submit another with the changed name and fixed CS violations?

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

by fabpot at 2012-03-12T07:41:11Z

`initialized()` looks fine to me. Make your changes, squash your commits and then force the push to your branch (the PR will be updated automatically).

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

by evillemez at 2012-03-12T20:49:17Z

I was about to squash my commits to update this, but it just occurred to me that I hadn't considered the interface.  Does anyone feel this method `initialized()` should be defined in ContainerInterface as well?  It seems like it's generic enough that it would make sense.  But I'm not immediately aware if this would cause BC breaks.

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

by fabpot at 2012-03-13T11:34:33Z

We cannot break BC for `ContainerInterface` as this is marked with the `@api` tag.

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

by henrikbjorn at 2012-03-13T12:34:42Z

Is it a BC break if we add a method? i thought only changing the already written methods would be a BC break.

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

by ooflorent at 2012-03-13T12:39:44Z

@henrikbjorn It will raise a fatal error if the method isn't implemented in existing class.

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

by lsmith77 at 2012-03-13T13:06:26Z

we could however add a new interface that extends from the previous one.

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

by evillemez at 2012-03-13T15:40:39Z

Are the BC breaks we are worried about for compatibility just within the Symfony repo - or in general in case others have implemented the interface elsewhere?  As far as Symfony is concerned, the only class I can find that implements `ContainerInterface` is `Container`, so adding the method shouldn't be an issue.

If it's an issue of principle, in case others may have implemented the `ContainerInterface`, then... yeah, it's a break, and maybe should be left out.  I suppose this would mean that other places in code that type hint for `ContainerInterface` would have to change the type hint to `Container` if they want to implement the `initialized()` method.

Is this more or less acceptable than updating the interface?

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

by evillemez at 2012-03-14T19:17:27Z

Hadn't properly squashed commits, just fixed.

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

by evillemez at 2012-03-15T14:06:38Z

I'm done with this PR, unless there is a consensus that I should also implement `Container::initialized()` in `ContainerInterface`.

Anything else I need to do?

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

by Seldaek at 2012-03-15T15:41:44Z

@evillemez the common pattern for BC is to add a new interface, say IntrospectableContainerInterface or something, that extends ContainerInterface, then you can type-hint that without restricting use of competing implementations of the Container class.

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

by stof at 2012-04-03T22:30:51Z

@evillemez Please update this PR. It conflicts with master because of the move of tests. And the new interface suggested by @Seldaek is a good idea IMO

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

by evillemez at 2012-04-04T14:57:29Z

@Stof I may not be able to get to this until the end of the week, but I'll do it as soon as I can.

I'll rebase against the current master, and add/implement the interface.  Is `IntrospectableContainerInterface ` as suggested by @Seldaek ok with everyone?

Are there other features that we can think of that would be useful for this new interface?  I don't want to start a precedent of adding a new interface for every new method that comes up... I understand it makes sense for not breaking backwards compatibility for something previously marked as stable, but still, it's yet another file that will likely be included on every request.

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

by stof at 2012-04-04T15:35:15Z

@evillemez classes used on every requests can be added to some cached bootstrap files (which are loaded during the ``$kernel->loadClassCache()`` call in your front controller) to avoid including many files through the autoloader. And for even better performances in prod, the solution is to use APC with ``apc.stat = 0``, as advocated by @lsmith77

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

by evillemez at 2012-04-15T19:00:07Z

Ok, rebased against current master and implemented the interface.  I didn't mark it as `@api` because I think we may want to consider if there's any other functionality that could be implemented there before declaring it stable.

Sorry for the delay.  My wife and I are in the process of buying a house, and some unexpected things have come up.

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

by fabpot at 2012-04-18T10:59:01Z

Can you rebase before I merge? Thanks.
2012-04-19 11:56:35 +02:00
Jordi Boggiano
725423c4c4 Add test to prevent regressions 2012-04-19 11:54:19 +02:00
Fabien Potencier
d599442cc1 merged branch jfsimon/issue-3896 (PR #4001)
Commits
-------

cd783fb [HttpKernel] Fixed cache vary lookup (fixes #3896).

Discussion
----------

[HttpKernel] Fixed cache vary lookup (fixes #3896).

Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #3896
2012-04-19 11:49:29 +02:00
Jordi Boggiano
3b2f5428c3 Fix asset generation with an empty asset 2012-04-19 11:46:54 +02:00