[2.2] [FrameworkBundle][Translation] Added logging capability to translator #5537
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
|
See also discussion in referenced PR. |
| 8000 | * @param string $domain | |
| * @return MessageCatalogueInterface | ||
| */ | ||
| protected function findCatalogueForId(MessageCatalogueInterface $catalogue, $id, $domain) |
There was a problem hiding this comment.
I still find the naming weird. This method does not find the catalogue as you are already finding it before that to pass it as an argument
There was a problem hiding this comment.
And btw, it does return a catalogue to allow for further potential customization.
There was a problem hiding this comment.
yes, it returns a catalogue. but it does not find it. All places using find as naming are actually finding the object. Here, you are eventually filtering the catalogue, but finding it is done before calling the method
There was a problem hiding this comment.
Ok, filterCatalogueForId is fine.
|
and why not adding the logging support in the component itself ? It would only be an optional dependency as the logger is optional. |
|
IMO, logging capability does not make much sense in the standalone component. I know it would be an optional dependency, but I still think it's leaner this way. |
|
why wouldn't it make sense for people using the Translator without the fullstack framework (in Silex for instance) ? |
|
I didn't say that, I said IMO an (even optional) dependency is overkill to solve a very specialized problem. |
|
@meandmymonkey an optional dependency on the logger will not hurt (if you don't like it, don't set a logger). But I think it is worth implementing the feature for everyone (Silex uses the translator component without using FrameworkBundle for instance) |
| { | ||
| if (null !== $this->logger && !$catalogue->defines($id, $domain)) { | ||
| if ($catalogue->has($id, $domain)) { | ||
| $this->logger->notice('Translator: using fallback catalogue.', array('id' => $id, 'domain' => $domain)); |
There was a problem hiding this comment.
I don't think it should be a notice. The fallback from fr_FR to fr is totally normal for instance, and this would be spamming your logs.
|
Can you also pass a locale to the logger same way as ID and domain is passed? Thanks. |
This PR was merged into the master branch. Commits ------- b930066 Add Route::hasOption() and Route::hasRequirement() methods. Discussion ---------- Add Route::hasOption() and Route::hasRequirement() methods. It seems odd that there's a hasDefault() method but not for options and requirements. Drupal has logic that depends on the existence of a given requirement. So let's add that. I think I got the coding standards right the first time for once... :-)
This PR was merged into the master branch. Commits ------- e2a50ef [OptionsResolver] fix normalizer without corresponding option 5a53821 [OptionsResolver] fix removing normalizers Discussion ---------- OptionsResolver: normalizer fix setNormalizer() -> replace() -> all() would generate an error. --------------------------------------------------------------------------- by bschussek at 2012-07-29T16:09:20Z Thank you for the fix! Could you please add a test case? --------------------------------------------------------------------------- by Tobion at 2012-07-30T15:42:26Z There is another problem: setNormalizer() (without setting an option) -> all() I suggest to simply ignore normalizers that have no corresponding option. Do you agree? --------------------------------------------------------------------------- by Tobion at 2012-07-30T16:19:24Z On the other hand, one could argue that a normalizer without option should also work like this: ``` $this->options->setNormalizer('foo', function (Options $options) { return ''; }); $this->assertEquals(array('foo' => ''), $this->options->all()); ``` But when having a normalizer that wants a previous value as param, it does not work (because there is none). --------------------------------------------------------------------------- by stof at 2012-07-30T16:30:34Z @Tobion according to github, this need to be rebased --------------------------------------------------------------------------- by bschussek at 2012-07-30T19:16:48Z I guess setNormalizer() should check whether the option is set and fail otherwise. The second possibility, as you say, is to ignore them in all(). I'd prefer whatever is more efficient. --------------------------------------------------------------------------- by bschussek at 2012-07-30T19:17:27Z But setting a normalizer without setting an option, and having that option appear in the final options, does not make sense if you ask me. --------------------------------------------------------------------------- by Tobion at 2012-07-30T21:23:46Z Well it could make sense. If you want to override/normalize an option to a given value however it has been overloaded by others or just not overloaded at all. This is what normalizers do. I think its more consistent than the other solutions. Raising exception in setNormalizer would make the Class dependent on the order you call the methods, e.g. `setNormalizer(); set()` would not work. But the other way round would be ok. Ignoring some normalizers in `all` would be strange because they are there but not applied under some circumstances. --------------------------------------------------------------------------- by Tobion at 2012-07-30T21:42:40Z Added the fix. If you disagree tell me. --------------------------------------------------------------------------- by bschussek at 2012-08-04T09:30:18Z > Raising exception in setNormalizer would make the Class dependent on the order you call the methods, e.g. `setNormalizer(); set()` would not work. But the other way round would be ok. I think this would be a better solution. I dislike if the normalizer magically adds an option that does not exist. This could hide implementation error, e.g. when a refactoring removes an option, but the normalizer is forgotten. Can you throw an exception in this case? Should we find use cases that rely on this to work, we can soften the behavior and remove the exception. --------------------------------------------------------------------------- by Tobion at 2012-08-04T15:02:51Z Well, that would also make it impossible to set a normalizer for on optional option in OptionsResolver. So `setOptional` + `setNormalizers` would throw an exception which sounds counter-intuitive. Are you sure about that? --------------------------------------------------------------------------- by Tobion at 2012-08-17T11:47:58Z ping @bschussek --------------------------------------------------------------------------- by Tobion at 2012-10-07T22:31:44Z @bschussek ping --------------------------------------------------------------------------- by stof at 2012-10-13T18:04:30Z @bschussek ping --------------------------------------------------------------------------- by Tobion at 2012-11-08T09:55:15Z @bschussek please let's get this finished.
…Visitor design pattern. With this refactoring comes a decoupling of the validator from the structure of the underlying metadata. This way it is possible for Drupal to use the validator for validating their Entity API by using their own metadata layer, which is not modeled as classes and properties/getter methods.
This PR was squashed before being merged into the master branch (closes #6080). Commits ------- e477a2e Handle case of static controller method and controllers using magic __call() method Discussion ---------- Handle case of static controller method and controllers using magic __call() method Improve collecting controller details for edge cases where: - controller is array, but contains class name and static method - method doesn't exist, but is handled by magic __call() method --------------------------------------------------------------------------- by fabpot at 2012-11-21T08:12:08Z Can you add some unit tests? --------------------------------------------------------------------------- by sli-systems at 2012-11-21T22:19:17Z @pierredup I disagree with the your comment about is_callable() only working with objects. The PHP docs state that the first argument is a callable, so it can be a string, array, closure, and perhaps more. The test I added also shows that the code works as is :) I've thought about your suggestion of adding reflection to look up the location of __call(). However, I think this doesn't really add a lot and only complicates matters. Also, as you can see in the new test, there is also __callStatic() to consider. The fact that file/line are n/a is correct, because the most typical case will be that __call() and __callStatic() will delegate to some other method that might not even be in the same class/file (a subclass I would expect), IMHO. @fabpot Good catch about the '/'. I hope the test is complete enough. Looks more like an exercise on PHP callables than anything else, tho ;) --------------------------------------------------------------------------- by pierredup at 2012-11-22T04:56:18Z True that ````is_callable```` takes any callable argument, except in the one specific case where you have a ````__call()```` method, and pass an array with the first paramater as a string. Take the following example: class Controller { public function __call($method, $arguments) {} } $controller = array('Controller', 'action'); var_dump(is_callable($controller)); Here ````is_callable($controller)```` will actually return ````false````, where if you have ````$controller = array(new Controller, 'action');```` it would return true. Of course if you have a ````__callStatic```` method, then it would always return true. Your tests doesn't seem to cover this use case --------------------------------------------------------------------------- by sli-systems at 2012-11-22T20:27:05Z Hmm, maybe. I have to admin that I do not know about this case. OTOH, if is_callable returns false is it really callable then? I would think this more of a PHP bug then? I think I might have come across this case during coding, but then dismissed it because in that case FilterControllerEvent failed already before the data collector code is reached. In FilterControllerEvent there is a check on is_callable and a LogicException is thrown if $controller is not callable. So, is FilterControllerEvent wrong too then? --------------------------------------------------------------------------- by pierredup at 2012-11-22T20:41:14Z One would think that if is_callable returns false, then the controller isn't callable, but in the case I mentioned above, the controller is in fact callable. I also thought it was a bug with php, but the php-internals don't seem to think so. The problem is, if you specify the class as a string, php looks for a static method, even if you have a __call method, it won't be registered. I will have a look at the FilterControllerEvent to see if this use case applies there as well. --------------------------------------------------------------------------- by sli-systems at 2012-11-22T20:50:32Z Rather strange - if that is the case then using is_callable seems pretty pointless and the only way would be to try to execute the controller to find out if it is, in fact, callable... --------------------------------------------------------------------------- by pierredup at 2012-11-22T20:51:07Z Okay so it actually seems that the case above isn't callable after all. If the controller is specified as a string, then a static method need to exist. Hence why it works with __callStatic. Only when an instance of the class is specified, will it handle the __call method. --------------------------------------------------------------------------- by sli-systems at 2012-11-22T20:57:55Z So the tests are sufficient then? --------------------------------------------------------------------------- by pierredup at 2012-11-22T20:59:22Z Yes it is. This happens when you just assume something without actually testing it :) Sorry for the hassle
This PR was merged into the master branch. Commits ------- d1b5093 Try to make sure cookies get deleted from the TokenProvider when no longer in use Discussion ---------- Delete cookies from the TokenProvider that is no longer in use Bug fix: yes Feature addition: no Backwards compatibility break: no Symfony2 tests pass: yes Todo: - License of the code: MIT When the user logs in, or login fails for some reason, the old "remember me" cookie should be deleted from the TokenProvider if you are using the PersistentTokenBasedRememberMeServices. As the code is now, the token is only deleted on logout. --------------------------------------------------------------------------- by TerjeBr at 2012-11-20T13:45:54Z So, anything else that needs to be done before this is merged? --------------------------------------------------------------------------- by TerjeBr at 2012-11-21T10:30:53Z Ok, I have corrected the typo in the comment and squashed the commit. --------------------------------------------------------------------------- by schmittjoh at 2012-11-21T10:36:29Z btw, ``canceled`` (more American) and ``cancelled`` (more British) are both correct English forms. On Wed, Nov 21, 2012 at 11:30 AM, Terje Bråten <notifications@github.com>wrote: > Ok, I have corrected the typo in the comment and squashed the commit. > > — > Reply to this email directly or view it on GitHub<#6055 (comment)>. > > --------------------------------------------------------------------------- by schmittjoh at 2012-11-21T10:40:24Z As a side-note have you verified that this does not break the cookie theft protection? --------------------------------------------------------------------------- by TerjeBr at 2012-11-21T10:51:10Z Yes, cookie theft protection is still there and is functioning well. --------------------------------------------------------------------------- by TerjeBr at 2012-11-21T11:14:04Z I am using this together with the DoctrineTokenProvider in issue #6057 in my own project and done some extensive testing on it. --------------------------------------------------------------------------- by TerjeBr at 2012-11-23T10:30:34Z Is this ready to be merged now?
This PR was merged into the master branch. Commits ------- 1858b96 [Form] Adapted FormValidator to latest changes in the Validator 1f752e8 [DoctrineBridge] Adapted UniqueValidator to latest changes in the Validator efe42cb [Validator] Refactored the GraphWalker into an implementation of the Visitor design pattern. Discussion ---------- [Validator] Refactored the Validator for use in Drupal Bug fix: no Feature addition: no Backwards compatibility break: yes Symfony2 tests pass: yes Fixes the following tickets: - Todo: - License of the code: MIT Documentation PR: TODO Drupal wants to use the Symfony Validator component in their next version. I was talking to @fago recently about the changes that we'd need to make and implemented these changes in this PR. I don't want to rush this, but the deadline is tight, since Drupal feature freeze is on December 1st and @fago needs at least a couple of days to integrate the Validator into Drupal. This PR introduces two significant changes: * Interfaces were created for all classes that constitute the Validator's API. This is were the PR breaks BC, because `ConstraintValidatorInterface::initialize()` is now type hinted against `ExecutionContextInterface` instead of `ExecutionContext`. * The graph walker was refactored into an implementation of the Visitor pattern. This way, the validator was decoupled from the structure of the metadata (class → properties and getter methods) and makes it possible to implement a different metadata structure, as is required by the Drupal Entity API. As a consequence of the API change, custom validation code is now much easier to write, because `ValidatorInterface` and `ExecutionContextInterface` share the following set of methods: ```php interface ValidatorInterface { public function validate($value, $groups = null, $traverse = false, $deep = false); public function validateValue($value, $constraints, $groups = null); public function getMetadataFor($value); } interface ExecutionContextInterface { public function validate($value, $subPath = '', $groups = null, $traverse = false, $deep = false); public function validateValue($value, $constraints, $subPath = '', $groups = null); public function getMetadataFor($value); } ``` No more juggling with property paths, no more fiddling with the graph walker. Just call on the execution context what you'd call on the validator and you're done. There are two controversial things to discuss and decide (cc @fabpot): * I moved the `@api` tags of all implementations to the respective interfaces. Is this ok? * I would like to deprecate `ValidatorInterface::getMetadataFactory()` (tagged as `@api`) in favor of the added `ValidatorInterface::getMetadataFor()`, which offers the exact same functionality, but with a different API and better encapsulation, which makes it easier to maintain for us. We can tag `getMetadataFor()` as `@api`, as I don't expect it to change. Can we do this or should we leave the old method in? I would like to decide the major issues of this PR until **Sunday November 25th** in order to give @fago enough room for his implementation. Let me hear your thoughts.
added 2 commits
November 24, 2012 14:24
…key/symfony into translator-log-catalog-misses
|
@slde-incal Apparently, many non-related commits have been introduced in this PR. Can you fix that? |
|
@fabpot Whoa, were did that come from... sure I will try to fix it as soon as possible... probably not today, though. |
|
@meandmymonkey any news about this? If not, I'll try to redo this in time for 2.2 if @fabpot would be ok with this to be merged for it. |
|
Any news on this PR? @meandmymonkey Will you have some time to finish your implementation? |
|
@fabpot Not until a few days after easter. If that's too long,I can close the PR. |
|
It looks like the PR contains some unrelated commits. And now that we use PSR-3 for the logger, it makes even more sense to move the logging capabilities to the component translator rather than the bundle one, so that people using the component standalone can also benefit from it. |
|
@stof Yes, agreed. |
|
Closing this PR as it is borked now and needs to be rewritten to take into account PSR-3 anyway. Feel free to reopen a new one. |
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: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #3015
Todo: -
Adds logging capabilities to
Symfony\Bundle\FrameworkBundle\Translation\Translator. Logging is switchable through the newloggingconfig flag and defaults tofalse.A minor addition to the Translation Component was necessary to avoid complete duplication of
trans()andtransChoice().