Commits
-------
060d75d [Console] fixed CS and simplified code
a5ff635 [Console] added support for interactive shell without readline extension
Discussion
----------
[2.1] [Console] added support for interactive shell without readline extension
---------------------------------------------------------------------------
by lmcd at 2011/06/30 15:12:56 -0700
Very useful! Pre-installed PHP on OS X ships without readline.
---------------------------------------------------------------------------
by stof at 2011/06/30 15:24:47 -0700
@lmcd You can already use the console, just not the shell.
---------------------------------------------------------------------------
by lmcd at 2011/06/30 15:27:00 -0700
@stof Yep I know, but it would be nice to use the shell :)
---------------------------------------------------------------------------
by hason at 2011/07/01 01:54:34 -0700
@stof Interactive generators will not work on Windows and Mac OS X without these simple changes. I think this is bad news.
---------------------------------------------------------------------------
by Seldaek at 2011/07/01 01:58:59 -0700
Yup, it's definitely a good addition.
---------------------------------------------------------------------------
by Seldaek at 2011/07/01 01:59:26 -0700
@fabpot: I'm not sure why this is tagged 2.1, but IMO this should go in now.
---------------------------------------------------------------------------
by stof at 2011/07/01 01:59:44 -0700
@hason This is not true. The shell is only used when running the console with the ``-s`` option. A command can be interactive without using the shell (I'm on Windows)
---------------------------------------------------------------------------
by lmcd at 2011/07/01 02:08:17 -0700
@stof Yea true, the interactive generators are fine without readline, however I don't think this dependancy should prevent people using the shell on Win/Mac.
---------------------------------------------------------------------------
by hason at 2011/07/01 02:28:10 -0700
@stof This is good news :) I was hasty.
---------------------------------------------------------------------------
by lmcd at 2011/07/04 02:03:36 -0700
I'm feeling inclined to remove readline entirely for 2.1 and roll our own solution to support things like this in the shell: http://lmcd.me/v/sf-autocomplete-2.mov
---------------------------------------------------------------------------
by stof at 2011/09/04 04:55:37 -0700
what is the status of this PR ?
Commits
-------
fd00ed0 [Console] Check if dialog helper actually exists
0f7bf41 [Console] Detect if interactive mode is possible at all
Discussion
----------
[Console] Detect if interactive mode is possible at all
---------------------------------------------------------------------------
by fabpot at 2011/07/19 22:52:14 -0700
You can use the dialog helper without a tty (think unit tests). see https://github.com/sensio/SensioGeneratorBundle/blob/master/Tests/Command/GenerateDoctrineCrudCommandTest.php#L24
---------------------------------------------------------------------------
by lenar at 2011/07/25 06:18:48 -0700
@fabpot: this change doesn't prevent those tests from running (and successfully so). Please reconsider including.
---------------------------------------------------------------------------
by lenar at 2011/07/27 01:30:20 -0700
Rebased.
Commits
-------
5146a1f [EventDispatcher] Added possibility for subscribers to subscribe several times for same event
Discussion
----------
[EventDispatcher] Added possibility for subscribers to subscribe several times for same event
[EventDispatcher] Added possibility for subscribers to subscribe several times for same event
closes#2146
And it is of course fully BC :)
---------------------------------------------------------------------------
by jalliot at 2011/09/09 17:34:07 -0700
If merged, #2021 will have to reflect the change too
Commits
-------
dd20f01 Fixed assets:install to use a relative path instead of an absolute
Discussion
----------
[2.1] Fixed "assets:install" to create relative instead of absolute symlinks
This is a fairly simple fix so that the symlinks are relative to the resources rather than an absolute path that breaks from machine-to-machine or upon deployment.
We were trying to figure out why styles were messed up for other contributors, but then found that the paths were hard-coded for my machine :)
---------------------------------------------------------------------------
by ericclemmons at 2011/06/04 09:44:11 -0700
Any other thoughts/updates on this?
---------------------------------------------------------------------------
by fabpot at 2011/06/04 22:31:39 -0700
We have such a feature in symfony1 and IIRC it does not work very well. One problem is when you use symlink for some bundles. Then, you should not use a relative symlink as there is a common path between the two.
---------------------------------------------------------------------------
by ericclemmons at 2011/06/05 09:55:00 -0700
Sorry, I didn't think that we would be an issue since the Bundle "Best Practices" states to not include other bundles as dependencies.
If absolute links are a must, then the next alternative is for collaborators to add "/web/bundles" to .gitignore and each person run "assets:install" upon installation/update.
I was personally hoping there were a way to have this versioned for easier deployment.
On Jun 5, 2011, at 12:31 AM, fabpot<reply@reply.github.com> wrote:
> We have such a feature in symfony1 and IIRC it does not work very well. One problem is when you use symlink for some bundles. Then, you should not use a relative symlink as there is a common path between the two.
>
> --
> Reply to this email directly or view it on GitHub:
> https://github.com/symfony/symfony/pull/1173#issuecomment-1303600
---------------------------------------------------------------------------
by henrikbjorn at 2011/06/27 04:56:58 -0700
``` php
<?php
// ...
->addOption('relative', null, InputOption::VALUE_NONE, 'The --symlink option will generate relative paths')
// ...
```
and just default to absolute paths ?
---------------------------------------------------------------------------
by ericclemmons at 2011/06/27 08:37:50 -0700
I'm very supportive of that compromise. Up to @fabpot if I should add this back in, since relative paths were apparently problematic with symfony1.
---------------------------------------------------------------------------
by sbusch at 2011/07/15 08:46:01 -0700
+1
I'm developing on Mac and the files are mounted on a Linux box which serves the project. The paths are not the same on those two systems. If I accidentally install assets on my Mac the absolute paths won't work on the Linux webserver.
Other scenario: one teammate could add those symlinks by accident to the git repository, which breaks all other installations.
Relative symlinks could help a lot in these cases.
---------------------------------------------------------------------------
by ericclemmons at 2011/07/15 08:47:53 -0700
@sbusch Your issues are the same as mine, which prompted this ticket :)
Until this gets @fabpot's blessing, it's best to simply add `web/bundles` to your `.gitignore` file and tell your users to always run `assets:install --symlink` each time they pull down code & something breaks ;)
---------------------------------------------------------------------------
by sbusch at 2011/07/15 08:58:33 -0700
The handling (calculation) of relative symlinks IMO fits better to the `symlink()` method of `\Symfony\Component\HttpKernel\Util\Filesystem`. Possible method signature:
symfony/src/Symfony/Component/HttpKernel/Util/Filesystem.php:
```php
<?php
// ...
/**
* Creates a symbolic link or copy a directory.
*
* @param string $originDir The origin directory path
* @param string $targetDir The symbolic link name
* @param Boolean $copyOnWindows Whether to copy files if on Windows
* @param Boolean $makeRelative Whether to try to create a relative link
*/
public function symlink($originDir, $targetDir, $copyOnWindows = false, $makeRelative = false)
{
```
And what about changing the `--symlink` option to optionally have a value, instead of adding a new depending option? E.g. `--symlink[=absolute|relative]`, with "absolute" as default:
symfony/src/Symfony/Bundle/FrameworkBundle/Command/AssetsInstallCommand.php:
```php
<?php
// ...
class AssetsInstallCommand extends ContainerAwareCommand
{
/**
* @see Command
*/
protected function configure()
{
$this
->setDefinition(array(
new InputArgument('target', InputArgument::REQUIRED, 'The target directory (usually "web")'),
))
->addOption('symlink', null, InputOption::VALUE_OPTIONAL, 'Symlinks the assets instead of copying it. Allowed values: "absolute" (default) and "relative".', 'absolute')
->setHelp(<<<EOT
The <info>assets:install</info> command installs bundle assets into a given
directory (e.g. the web directory).
<info>./app/console assets:install web [--symlink]</info>
A "bundles" directory will be created inside the target directory, and the
"Resources/public" directory of each bundle will be copied into it.
To create a symlink to each bundle instead of copying its assets, use the
<info>--symlink</info> option. Use <info>--symlink=relative</info> for relative symlinks.
EOT
)
->setName('assets:install')
;
}
/**
* @see Command
*
* @throws \InvalidArgumentException When the target directory does not exist
*/
protected function execute(InputInterface $input, OutputInterface $output)
{
if (!is_dir($input->getArgument('target'))) {
throw new \InvalidArgumentException(sprintf('The target directory "%s" does not exist.', $input->getArgument('target')));
}
if ($input->hasOption('symlink'))
{
if (!function_exists('symlink')) {
throw new \InvalidArgumentException('The symlink() function is not available on your system. You need to install the assets without the --symlink option.');
}
if (!in_array($input->getOption('symlink'), array('absolute', 'relative'))) {
throw new \InvalidArgumentException(sprintf('Invalid value "%s" for option "symlink"', $input->getOption('symlink')));
}
}
$filesystem = $this->getContainer()->get('filesystem');
// Create the bundles directory otherwise symlink will fail.
$filesystem->mkdir($input->getArgument('target').'/bundles/', 0777);
foreach ($this->getContainer()->get('kernel')->getBundles() as $bundle) {
$originDir = $bundle->getPath().'/Resources/public';
if (is_dir($originDir)) {
$targetDir = $input->getArgument('target').'/bundles/'.preg_replace('/bundle$/', '', strtolower($bundle->getName()));
$output->writeln(sprintf('Installing assets for <comment>%s</comment> into <comment>%s</comment>', $bundle->getNamespace(), $targetDir));
$filesystem->remove($targetDir);
if ($input->hasOption('symlink')) {
$filesystem->symlink($originDir, $targetDir, false, $input->getOption('symlink') == 'relative');
} else {
$filesystem->mkdir($targetDir, 0777);
$filesystem->mirror($originDir, $targetDir);
}
}
}
}
```
---------------------------------------------------------------------------
by sbusch at 2011/07/15 09:04:46 -0700
@ericclemmons: yes, that's our current workaround. I started with manually converting absolute links to relative ones, but that quickly got very annoying ;-)
After that I tried to implement the generation of relative links by myself (where the proposals of my previous comment come from) until I found your PR.
---------------------------------------------------------------------------
by henrikbjorn at 2011/07/18 00:20:38 -0700
@sbush if it defaults to something how would you turn it off ?
---------------------------------------------------------------------------
by stof at 2011/07/18 00:26:16 -0700
@henrikbjorn a default value for the option is used when using ``--symlink`` without the value. If you don't use the option at all, it is disabled.
---------------------------------------------------------------------------
by stof at 2011/07/18 11:58:29 -0700
In fact no. The default value seems to be also used when the option is not set at all. @fabpot is this intended ?
---------------------------------------------------------------------------
by Seldaek at 2011/07/19 05:18:29 -0700
Symlinks on windows, although technically possible, don't quite work with PHP on most setups. Also git doesn't seem to support them either on windows (not sure why not). For those reasons, and although I'm sure this doesn't apply to every project, I would recommend you just have everyone run `assets:install [--symlink]` on their local machine, and make that command run on the server as part of your deployment process.
---------------------------------------------------------------------------
by ericclemmons at 2011/07/19 06:15:34 -0700
Nobody is even entertaining --relative?
On Jul 19, 2011, at 7:18 AM, Seldaek<reply@reply.github.com> wrote:
> Symlinks on windows, although technically possible, don't quite work with PHP on most setups. Also git doesn't seem to support them either on windows (not sure why not). For those reasons, and although I'm sure this doesn't apply to every project, I would recommend you just have everyone run `assets:install [--symlink]` on their local machine, and make that command run on the server as part of your deployment process.
>
> --
> Reply to this email directly or view it on GitHub:
> https://github.com/symfony/symfony/pull/1173#issuecomment-1606463
---------------------------------------------------------------------------
by Gregwar at 2011/08/10 08:56:27 -0700
I agree with the idea of proposing a --relative option, I'm currently working on a Samba mounted filesystem and I'm forced to create manually symlinks to get things working since the paths are not the same
Commits
-------
908a7a3 [HttpFoundation] Fix bug in clearCookie/removeCookie not clearing cookies set with a default '/' path, unless it was explicitly specified
Discussion
----------
[HttpFoundation] Fix bug in clearCookie/removeCookie not clearing cookies
[HttpFoundation] Fix bug in clearCookie/removeCookie not clearing cookies set with a default '/' path, unless it was explicitly specified
---------------------------------------------------------------------------
by Seldaek at 2011/08/02 10:31:44 -0700
The reason is that Cookie::__construct defaults to '/' btw, so if you don't specify it, and then call clearCookie without specifying again, the paths don't match.
---------------------------------------------------------------------------
by Koc at 2011/08/07 00:06:13 -0700
I think that correctrly use base path. Is it possible?
For example we have 2 apps
* site.com/app1/index.php
* site.com/app2/index.php
and app2 will remove cookies of app1
---------------------------------------------------------------------------
by Seldaek at 2011/08/07 02:58:10 -0700
IMO if people want that they should specify the path manually, by default cookies are always set for the entire host and I think it should stay like that.
---------------------------------------------------------------------------
by Koc at 2011/08/07 04:26:47 -0700
It is hard to specify path manually everywhere when set/remove cookies.
---------------------------------------------------------------------------
by Seldaek at 2011/09/27 07:01:43 -0700
@fabpot: ping? You said this was ok, but it was never merged.
Commits
-------
f7bf7b5 fixed condition
181332b added a Controller:getUser() shortcut to recover the current user
Discussion
----------
[2.1] added a Controller:getUser() shortcut to recover the current user
Commits
-------
c4a0f79 Updates according to suggestions.
6aec789 Added tests.
54454ba Added generic filtering to ParameterBag.
Discussion
----------
Added generic filtering to ParameterBag.
Adds filtering convenience using PHP's filter_var() e.g.
$request->get->filter($key, '', false, FITLER_SANITIZE_STRING);
See http://php.net/manual/en/filter.filters.php for capabilities.
---------------------------------------------------------------------------
by GromNaN at 2011/09/25 15:41:50 -0700
What is the use case ?
---------------------------------------------------------------------------
by drak at 2011/09/25 15:52:19 -0700
Input variable validation/sanitization. ParameterBag has a few built in like `getAlnum()` for example. This method offer's PHP's full filtering and sanitization suite.
---------------------------------------------------------------------------
by fabpot at 2011/09/27 00:56:41 -0700
Can you add some unit tests for this new feature?
---------------------------------------------------------------------------
by drak at 2011/09/27 00:58:56 -0700
Sure thing.
---------------------------------------------------------------------------
by drak at 2011/09/27 01:07:03 -0700
Before I make the commit, is the method name ok for you or would you prefer it is called `getFiltered()`?
---------------------------------------------------------------------------
by fabpot at 2011/09/27 01:13:46 -0700
`filter` sounds good to me.
---------------------------------------------------------------------------
by drak at 2011/09/27 02:37:01 -0700
I've added some tests.
---------------------------------------------------------------------------
by stloyd at 2011/09/27 02:42:42 -0700
@drak IMO you must check that user don't use unknown filter and/or flags for filter.
---------------------------------------------------------------------------
by drak at 2011/09/27 02:48:38 -0700
@stloyd - I'm not sure that's practical at all, this is a wrapper for a built-in PHP function and I don't understand why we would need validate arguments for a PHP function - it's the coder's job to use the API correctly - none of the inputs to this function are coming from a web request. It would also mean that the API would need to keep track of any upstream changes to constants in the PHP engine (which are just integers after all). It's really just not practical.
---------------------------------------------------------------------------
by stealth35 at 2011/09/27 05:16:50 -0700
@drak it's could be cool to use `filter_id` ✌️
if (is_string) {
$filter = filter_id($filter);
}
---------------------------------------------------------------------------
by drak at 2011/09/27 07:05:42 -0700
@stealth35 regarding this
if (is_string) {
$filter = filter_id($filter);
}
I believe strongly in the use of IDEs when coding and autocomplete nicely provides when you type `FILTER_`. Additionally, `filter_id()` only works on filters, but not for the flags, so I'm not entirely sure how useful it would be overall compared to using a good IDE (which you need when working with complex frameworks anyhow, imo :)
---------------------------------------------------------------------------
by drak at 2011/09/27 07:30:10 -0700
Ok check it now.
Commits
-------
d675c28 [FrameworkBundle] Use Router instead of RouterInterface
ae7ae8d [FrameworkBundle] Moved router_listener from web to router.xml since it depends on the router
35a9023 [FrameworkBundle] Added isEnabled to Router commands, fixes#1467536d979 [Console] Added Command::isEnabled method that defines whether to add the command or not
Discussion
----------
[2.1] [Console] Added Command::isEnabled method
This addresses #1467.
The idea is to allow commands to evaluate whether they can run or not, since they are automatically registered.
- It's useful for the two router:* commands since they're optional (router can be disabled), but part of the FrameworkBundle that is not really optional.
- It could be useful for third party code as well.
- It's BC.
- aa95bb0d395810b29a3e654673e130736d9d1080 should address the issue in #1467, while the other commits just make sure the command is not registered at all if the router isn't standard.
One issue remains though:
- A few other services like twig helpers get the `ròuter` injected, this means that if there is really **no** router service defined, there is still an error. I'm not sure how to fix those beyond adding `on-invalid="null"` but I'm not sure if that's desirable. I guess we could argue that the router is a big candidate for replacement/suppression, and as such it should be truly optional, but if we do it I don't know where it'll lead. I don't want to end up in a situation where half the dependencies are optional to support every possible combination. @fabpot wdyt?
---------------------------------------------------------------------------
by kriswallsmith at 2011/06/28 16:19:46 -0700
I'd rather see us not register a command instead of register and then disable it. Can we do the same thing you've done here in the bundle's registerCommands() method?
---------------------------------------------------------------------------
by Seldaek at 2011/06/28 16:51:36 -0700
Note that it's never really registered. During the registration it's checked and skipped if not enabled.
However, doing it as you suggest means overriding/copy-pasting all the code from the core Bundle class, which I don't like so much. It also means adding code specific to those two commands in a somewhat unrelated place, which I also don't like.
I'm not saying the current solution is perfect, but from the alternatives I considered, it's the best I have found.
---------------------------------------------------------------------------
by stof at 2011/09/04 04:58:04 -0700
@Seldaek your branch conflicts with master. could you rebase it ?
@fabpot what do you think about this PR ?
---------------------------------------------------------------------------
by Seldaek at 2011/09/04 08:39:05 -0700
Rebased
Commits
-------
6555e28 Remove unnecessary use statement
369f181 [FrameworkBundle] Add request scope to assets helper only if needed
d6b915a [FrameworkBundle] Assets templating helper does not need request scope
Discussion
----------
[FrameworkBundle] Assign request scope to assets helper only if needed
**Note**: This PR replaces #2218
I traced this request scope addition back to [an old commit](d9f5c99fab (commitcomment-597654)) from @kriswallsmith.
No other helpers have request scope and the assets helper's parameters don't appear to depend on the request in any way, so this appears to be unnecessary. As-is, request scope here prevents use of the assets helper from a console command that may need to internally render a template.
---------------------------------------------------------------------------
by fabpot at 2011/09/20 08:10:48 -0700
There is probably a reason why it was set to the request scope (just run the unit tests ;)).
---------------------------------------------------------------------------
by jmikola at 2011/09/20 08:22:46 -0700
Doh! Didn't even think to do that for such a small change.
I will investigate further. Do you have any idea how one can generate templates (which call the `asset()` function) during a console script?
---------------------------------------------------------------------------
by fabpot at 2011/09/20 08:33:22 -0700
Apparently, something depends on the Request somewhere. We need to find what needs the Request and determine if this is legitimate or not.
---------------------------------------------------------------------------
by henrikbjorn at 2011/09/20 09:25:05 -0700
https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Templating/Asset/PathPackage.php#L33
---------------------------------------------------------------------------
by jmikola at 2011/09/20 10:40:23 -0700
Thanks @henrikbjorn. So, it appears the default behavior of the asset helper is to use the request path as the base URL for assets. In my particular case, I didn't get this scope-widening exception at runtime because I use `assets_base_urls`, which results in the request-independent UrlPackage being injected instead of PathPackage.
This is tricky. I think there's definitely a use case for folks rendering templates outside of a request. Sending HTML emails via some console command is perhaps the most common example. Also, if those developers are not using a CDN, they likely won't think of using `assets_base_urls` at all. Unfortunately, that appears to be the only configuration option where the site's domain is specified (outside of, say, a session domain).
---------------------------------------------------------------------------
by lsmith77 at 2011/09/20 10:45:42 -0700
So do we need 2 services?
---------------------------------------------------------------------------
by jmikola at 2011/09/20 10:52:02 -0700
Well, we actually already have two services. I think that's why PathPackages and UrlPackages are distinct. The issue is that the asset helper has been assigned the request scope to accommodate to satisfy the most restrictive possibility.
I don't think two separate asset helpers would help the developer, even if it'd let us get passing tests.
Also, there is a very real use case here that I wasn't struggling with. You want to render templates with asset paths outside of a request (from a console command). In that case, you need a base URL, but where do you get that from?
Has anyone else encountered that requirement? I know we definitely did at OpenSky, and now at Exercise.com.
---------------------------------------------------------------------------
by jmikola at 2011/09/22 20:14:37 -0700
Had a chat with @kriswallsmith today. We brainstormed and came up with the idea of simply adding `strict="false"` to the request arguments of these templating services. That would disable the premature scope exception, but lead to different kind of error if the user attempted to call `asset()` when using a PathPackage (no `asset_base_urls` and no UrlPackage in play).
The PathPackage class in the templating component obviously has no request dependency -- just the FrameworkBundle version. I started modifying FrameworkBundle's PathPackage to make the Request usage optional (if it's not available, an empty base path is provided to the parent constructor, which actually works fine).
In the course of implementing that, I made the request argument `strict="false" on-invalid="null"` and encountered some strange behavior (possibly a bug) where the PathPackage service generated in FrameworkExtension via the DefinitionDecorator was not inheriting the strict/invalid properties. If I removed `on-invalid="null", strictness was inherited just fine. I'm going to continue looking into this, but at this point it seems like that may be a separate PR if it is, indeed, a bug.
Just going to paste my chat transcript with Kris here for some extra context:
```
<jmikola> if you have a moment later, could you chime in on https://github.com/symfony/symfony/pull/2223 ?
<jmikola> it's about using the asset() helper from a console command
<jmikola> i could have sworn we were sending emails like this in opensky, from console commands
<jmikola> doing the same thing for exercise, but the request scope on the asset helper is a cog in the gear
<kris> you don't have base urls configured?
<jmikola> we do
<jmikola> but the service is request-scoped because it doesn't know until runtime if it will be using UrlPackage or PathPackage
<jmikola> and PathPackage depends on the request object
<kris> yeah, it's tricky
<jmikola> so i think the scope was added to handle the most restrictive case
<jmikola> no way around that except to remove the scoping at runtime i suppose
<kris> we can tweak the scope in a compilation pass
<kris> but that could create some wtfs
<jmikola> why?
<jmikola> because some asset() calls might specify a package name?
<jmikola> without base_url's?
<kris> what about setting strict to false?
<jmikola> i haven't played with package names but i think that's how it works
<jmikola> ah, what's the problem with that again? i know it disables the exception
<jmikola> but if strict is false, what's the benefit of having request scope at all?
<kris> there are a lot of instances of strict="false" in the core
<jmikola> if strict is false, the scope becomes just documentation?
<jmikola> i'm drawing a blank on how that works - i just know strictness turns on the exceptions for widening violations
<kris> right
<kris> it means that helper would no longer be in the request scope, even though it uses a service from the request scope
<kris> i think that would be fine
<jmikola> ah, strict=false goes on the service argument
<jmikola> does that mean we'd apply that to the definition of templating.asset.path_package ?
<kris> yes
<jmikola> or just when PathPackage becomes an argument to the helper?
<jmikola> ok
<kris> whatever uses the request
<jmikola> and this would still cause havoc if I didn't define base_urls and tried to call asset() :)
<jmikola> i suppose in the form of null passed when Request instance expected?
<jmikola> or worse, undefined service request
<kris> right...
<jmikola> ok
<kris> so maybe we need a way to manually define the base path
<jmikola> for PathPackage
<kris> right, i think that's what's in the component
<jmikola> UrlPackage only comes into play if there's 1 or more assets_base_urls defined, right?
<kris> right
<jmikola> i'm looking at it, and am curious what happens if it was constructed with no base urls :)
<jmikola> since there's no checks that the array is full
<jmikola> *non-empty
<jmikola> ah, if it's empty it uses '' as the base Url
<jmikola> Perhaps PathPackage in FrameworkBundle could make the request optional
<jmikola> the core PathPackage does support an empty baseUrl
<jmikola> $basePath rather
<jmikola> it uses "/" by default
<kris> how about a configurator that calls setBasePath() if the container includes a request?
<jmikola> then the service would still need to be request scoped, no?
<jmikola> what it the same service was used on multiple requests
<jmikola> thinking of those trendy php app servers :)
<kris> the service wouldn't use the request
<kris> the configurator would
<kris> but via dic injection
<jmikola> do configurators exist?
<jmikola> i don't think i've seen one, a class with that name is in the DistributionBundle though - probably different
<kris> you can define a configurator on a service
<jmikola> example? is this done via tags?
<kris> it's a callable that will be passed the service before it leaves the container for the first time
<jmikola> so the PathPackage base class in the Templating component has a private basePath, it'd need a setBasePath method added of course
<kris> yeah, that's fine
<jmikola> ah <configurator
<jmikola> can't believe i've never seen this before
<kris> it's an old feature
<kris> and you can only set one for some reason
<jmikola> woah, it's only in test fixtures
<jmikola> nothing actually uses it
<jmikola> yet :)
<jmikola> configurators receive the container as their only argument?
<jmikola> oh, no arguments at all
<kris> they receive the service
<kris> but a configurator can be a service itself
<kris> i would add a base path node to the config
<kris> and disallow using both base path and base urls
<kris> and only use the configurator if the base path isn't explicitly set
<jmikola> now, in this case i'd have the configurator for the PathPackage service be another service that depends on request
<kris> no
<kris> the configurator should depend on the container
<kris> check if there is a request service and use it to set the base path if so
<jmikola> ok, and it's responsible for $pathPackage->setBaseUrl($request->getBasePath()) if the request exists
<jmikola> right
<kris> setBasePath
<jmikola> correct, sorry
<jmikola> i think a base_path and base_url can co-exist
<jmikola> base_url will entail use of the UrlPackage
<jmikola> and base_path is simply the default value for PathPackages
<jmikola> in the absence of the request
<kris> i don't follow
<jmikola> a base_url option in the config would simply be for the case where PathPackage is utilized outside of a request scope (i.e. console command)
<jmikola> sorry, base_path***
<jmikola> assets_base_url is for UrlPackages
<kris> right
<jmikola> ok, so you're saying at the top level, or in the packages prototypes, we'd allow only one or the other
<jmikola> at present, there's only asset_base_url's, so that implies configurations creating only UrlPackages
<kris> add a validation to the Configuration class so you have to choose one or the other
<kris> i don't understand why you would need base_path from the cli
<jmikola> so in this case, absence of either will result in a PathPackage configured based on the current request
<kris> don't you need absolute urls in your emails?
<kris> i'm rethinking
<jmikola> yes, we're using UrlPackages of course
<jmikola> but the request scope ruined this for me
<kris> don't bother with base_path in the config
<kris> just do the configurator
<jmikola> there's still a possibility that someone would generate HTML for on-site use from a console command
<jmikola> perhaps pre-building twig templates or something
<jmikola> your idea made sense i think
<jmikola> even if it's an edge case
<jmikola> granted, most people are perfectly fine with "/" as a default base_path in the absence of a request
<kris> that would be another pr
<jmikola> right
<jmikola> then for now, what about just making the request argument on-invalid null?
<jmikola> and changing PathPackage in the bundle
<jmikola> PathPackage in the component already handles an empty base path itself just fine
<kris> yeah, that's better
<jmikola> on-invalid="null" and strict="false"
<kris> nice
<jmikola> i'm satisifed with the simplicity of that
<jmikola> and will comment about your base_path option idea in my PR for a future task when i get around to it - as that sounded great as well
<jmikola> thanks for talking through this w/ me :)
<kris> np
<jmikola> PackageFactory is the new roadblock, heh
<jmikola> getPackage() { return $this->container->get($request->isSecure() ? $sslId : $httpId); }
<jmikola> :D
<kris> oh yeah
<jmikola> should i utilize the same logic? default to non-ssl?
<jmikola> i'm actually curious how gmail handles references to http:// assets embedded in its emails
<jmikola> i suppose it renders message bodies in iframes
<kris> then it would be impossible to render https asset urls without a request
<jmikola> wouldn't it default to the http url?
<kris> right
<kris> but what if you only have https base urls configured?
<jmikola> if you break them into http and https blocks, it'd be a problem
<kris> i think it always uses https if you only have https urls configured
<jmikola> otherwise, the shorthand notation sorts them for you
<jmikola> and all https urls are available to http
<jmikola> ah, you're correct
<jmikola> FrameworkExtension::createPackageDefinition
```
---------------------------------------------------------------------------
by jmikola at 2011/09/26 10:39:00 -0700
@fabpot: I believe I reached an agreeable solution, so please review when you get a chance.
I removed any `strict` and `on-invalid` trickery (ideas I was discussing with Kris) and decided to only apply request scoping to the assets helper if one or more of its injected packages (default or named) is request scoped.
This satisfies my requirements, as I had absolute base URL's defined for both HTTP and SSL protocols. It also leaves the current scope exceptions in place if, for instance, someone only defined an HTTP base URL and FrameworkBundle ended up creating a PathPackage for their SSL assets. I go into detail about this more in my second commit, and I think the added tests help explain this as well.
---------------------------------------------------------------------------
by jmikola at 2011/09/26 16:56:30 -0700
Something screwed up earlier and this PR's branch included all commits in `symfony/master`. I just cleaned things up.
Builds upon aead4a9836180cabae4d47fe27c634dcd79ac8f2, which prematurely removed request scoping from the assets templating helper in all cases. The helper need only be request-scoped if one or more request-scoped packages (e.g. PathPackages) are injected into it. This change makes it possible to utilize the assets helper outside of a request (e.g. during a console script).
To ensure that the assets helper is not assigned a request scope, all asset base URL's must be defined for all packages (default and any named) and both protocols: HTTP and SSL. The included test config fixtures concisely accomplish this by specifying a single HTTPS URL as the base URL for our default and named package, since FrameworkExtension's Configuration conveniently registers this URL for both protocols.
No other helpers have request scope and the assets helper's parameters don't appear to depend on the request in any way, so this appears to be unnecessary. As-is, request scope here prevents use of the assets helper from a console command that may need to internally render a template.
