Complete rewrote FrameworkBundle config reference #4736
Conversation
wouterj
force-pushed
the
framework_config
branch
from
2a587f1 to
847bcad
Compare
| .. tip:: | ||
|
|
||
| The XSD schema is available at | ||
| ``http://symfony.com/schema/dic/symfony/symfony-1.0.xsd``. |
There was a problem hiding this comment.
Are you sure that this file is up-to-date?
There was a problem hiding this comment.
I'm not sure about the uploaded file, but this is meant as a reference for when people want to use XML schema validation in Symfony (the local file is used for xsv in Symfony). That's also why it's a literal and not a link.
There was a problem hiding this comment.
Ah, I missed that you didn't link it.
|
@wouterj Could you split up the removal of the full config reference in a second commit? That would make it easier to review. Thanks. |
| // app/config/config.php | ||
| $container->loadFromExtension('framework', array( | ||
| 'trusted_proxies' => array('192.0.0.1', '10.0.0.0/8'), | ||
| )); |
There was a problem hiding this comment.
We have a challenge to not create any duplication. Since this is documented elsewhere, I would include the link (you have this) and not the code-block. Let's make the one place we have this documented really great. I would probably do the same with the CIDR notation, though I think if we just mentioned that in the description, it feels ok (e.g. ... configures the IP addresses (e.g. 192.0.0.1, 10.0.0.0/8)...). It's not a hard rule, just trying to have less.
|
Note to self: rebase to include #4711 and remove my own description |
|
@xabbuh done |
wouterj
force-pushed
the
framework_config
branch
from
847bcad to
8b820f4
Compare
| )); | ||
|
|
||
| Hosts can also be configured using regular expressions, which make it easier to | ||
| respond to any subdomain: |
There was a problem hiding this comment.
I would almost rather save us some space with code blocks and include the examples inline in the sentence:
Hosts can also be configured using regular expressions (e.g. ``.*\.?acme.com$`` or ``.*\.?acme.org$``), which make it ...
| `assets_version`_ option to construct an asset's path. By default, the pattern | ||
| adds the asset's version as a query string. For example, if | ||
| ``assets_version_format`` is set to ``%%s?version=%%s`` and ``assets_version`` | ||
| is set to ``5``, the asset's path would be ``/images/logo.png?version=5``. |
There was a problem hiding this comment.
They're all correct - it's a possessive apostrophe - the path belongs to the asset :)
|
Any news on this PR? Can I help you with anything? I'd love to see merged soon the great work done by @wouterj to document all this. |
|
@javiereguiluz time (and motivation to start fixing all these comments...). It's on my list for this week though, so I'll let you know if I need help if I don't get it finished this week. |
wouterj
force-pushed
the
framework_config
branch
from
8b820f4 to
b560fab
Compare
Conflicts: reference/configuration/framework.rst
wouterj
force-pushed
the
framework_config
branch
from
9b21ac0 to
bb9a1e7
Compare
|
I've applied all comments. There is only one drawback: I'm unable to make any sense of rebasing this PR. The config simply has too many changes and git isn't really able to know where to merge things, resulting in wrongly placed sections without a conflict (e.g. the csrf enabled setting under the routing setting). @weaverryan what do you think about splitting this PR up into a lot of small PRs, all adding one thing, slowly moving towards merging this complete PR? It will also be easier to take new settings into account in newer versions. If you agree with this approach, I'm willing to handle the opening and merging. (I need to be sure this PR is ready to merge in that case) |
|
@weaverryan can you please share your opinion about what I said above? |
|
I would prefer splitting this instead of throwing away all the work you did @wouterj. |
|
|
||
| .. note:: | ||
|
|
||
| This will automatically enable the validation. |
There was a problem hiding this comment.
Maybe make validation link to the validation section below?
|
👍 for the plan! I've just added some last comments, but unless you disagree with any of them (in that case, ping me), I have full confidence that you can make those tweaks in the new PR's and merge them without needing to wait for my review. Apologies if any delays from me made this too crazy to keep up to date. So yea, let's go with the plan! This is really great stuff! |
|
Continued in #5298 |
This PR was merged into the 2.3 branch. Discussion ---------- Completed framework config This PR only changes and adds new configuration, without moving all things around. This will things a lot easier to diff and see where things have gone wrong. I'll thouroughly review this PR and merge then. Replaces #4736 | Q | A | --- | --- | Doc fix? | yes | New docs? | yes | Applies to | 2.3+ | Fixed tickets | #4439, #3586 Commits ------- 54e8b0a Let docbot review reference f223643 Apply some of Ryan's suggestions 7e45e8f Completed framework config reference
Replaces #4121
Original PR body: