Integrates #2733 into master branch #2820
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…lways stores a string representation of each parameter
fabpot
added a commit
that referenced
this pull request
Dec 9, 2011
Commits ------- bb0d202 Switched sanitizeParameter() for existing varToString()-method; now always stores a string representation of each parameter 4fe4dfd Fixed vendor version mismatch in tests 28730e9 [DoctrineBridge] Added unit tests 4535abe [DoctrineBridge] Fixed attempt to serialize non-serializable values Discussion ---------- [DoctrineBridge] Fixed attempt to serialize non-serializable values Bug fix: yes Feature addition: no Backwards compatibility break: no (99%) Symfony2 tests pass: yes Fixes the following tickets: - Todo: - The Doctrine DBAL type system does not pose any restrictions on the php-types of parameters in queries. Hence one could write a doctrine-type that uses a resource or an `\SplFileInfo` as its corresponding php-type. Parameters of these types are logged in the `DoctrineDataCollector` however, which is then serialized in the profiler. Since resources or `\SplFileInfo` variables cannot be serialized this throws an exception. This PR fixes this problem (for known cases) by sanitizing the query parameters to only contain serializable types. The `isNotSerializable`-check surely is not complete yet, but more non-serializable classes can be added on a case-by-case basis. --------------------------------------------------------------------------- by fabpot at 2011/12/07 07:04:43 -0800 Tests do not pass for me. Furthermore, let's reuse what we already have in the framework (see https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/HttpKernel.php#L187 -- yes you can just copy/paster the existing code). --------------------------------------------------------------------------- by aboks at 2011/12/09 01:41:14 -0800 @fabpot I fixed the tests (seems I had the wrong vendor versions in my copy) and reused the `varToString()`-code. This introduces a tiny BC break in the rare case that someone writes his own templates for the web profiler (the parameters returned by the data collector are now always a string; could be any type before). After merging this PR, merging 2.0 into master would give a merge conflict and failing tests (because of the changes related to the introduction of the `ManagerRegistry` interface). To prevent this, please merge #2820 into master directly after merging this PR (so before merging 2.0 into master). After that 2.0 can be cleanly merged into master. --------------------------------------------------------------------------- by stof at 2011/12/09 03:43:38 -0800 it is not a BC break. Using ``yaml_encode`` on a string will not break the template
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Bug fix: yes
Feature addition: no
Backwards compatibility break: no (99%)
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: PR #2733 needs to be merged into 2.0 first
This PR fixes merge conflicts and failing tests that would otherwise arise if #2733 is merged into 2.0 and then 2.0 is merged into master. By first merging #2733 into 2.0, then directly merging this PR into master, 2.0 will merge cleanly into master afterwards.