Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor the form extractor to allow extraction from FormListeners #96

Merged
merged 1 commit into from
May 18, 2013
Merged

Refactor the form extractor to allow extraction from FormListeners #96

merged 1 commit into from
May 18, 2013

Conversation

acasademont
Copy link
Contributor

see #93

Also as said in that ticket, there are some tests failing due to the new PSR-3 LoggerInterface and SF 2.2 components

acasademont
acasademont reviewed Jan 14, 2013
View reviewed changes
Translation/Extractor/File/FormExtractor.php
@@ -272,8 +172,7 @@ private function parseItem($item, $domain = null)
// get doc comment
$ignore = false;
$desc = $meaning = null;
$docComment = $item->key->getDocComment();
$docComment = $docComment ? $docComment : $item->value->getDocComment();
$docComment = $item->value->getDocComment() ? $item->value->getDocComment() : $item->getDocComment();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems I did it the other way round. Any preferences for this?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say, let's keep the original order, no need to change this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@acasademont
Copy link
Contributor Author

@schmittjoh any more things that should be taken care of? I have been testing the patch and everything seems to be working

@schmittjoh
Copy link
Owner

Could you remove the minimum stability setting, and check whether the tests pass then?

@acasademont
Copy link
Contributor Author

There is one test still failing. It is failing in the current master branch too

PHPUnit 3.6.4 by Sebastian Bergmann.

Configuration read from /home/albert/projects/JMSTranslationBundle/phpunit.xml.dist

...............................................................  63 / 107 ( 58%)
................E...........................

Time: 1 second, Memory: 38.00Mb

There was 1 error:

1) JMS\TranslationBundle\Tests\Translation\Extractor\File\TwigFileExtractorTest::testExtractEdit
Twig_Error_Syntax: The function "path" does not exist in "/home/albert/projects/JMSTranslationBundle/Tests/Translation/Extractor/File/Fixture/edit.html.twig" at line 8

/home/albert/projects/JMSTranslationBundle/vendor/twig/twig/lib/Twig/ExpressionParser.php:547
/home/albert/projects/JMSTranslationBundle/vendor/twig/twig/lib/Twig/ExpressionParser.php:335
/home/albert/projects/JMSTranslationBundle/vendor/twig/twig/lib/Twig/ExpressionParser.php:148
/home/albert/projects/JMSTranslationBundle/vendor/twig/twig/lib/Twig/ExpressionParser.php:85
/home/albert/projects/JMSTranslationBundle/vendor/twig/twig/lib/Twig/ExpressionParser.php:42
/home/albert/projects/JMSTranslationBundle/vendor/twig/twig/lib/Twig/Parser.php:146
/home/albert/projects/JMSTranslationBundle/vendor/twig/twig/lib/Twig/TokenParser/Block.php:47
/home/albert/projects/JMSTranslationBundle/vendor/twig/twig/lib/Twig/Parser.php:192
/home/albert/projects/JMSTranslationBundle/vendor/twig/twig/lib/Twig/Parser.php:100
/home/albert/projects/JMSTranslationBundle/vendor/twig/twig/lib/Twig/Environment.php:480
/home/albert/projects/JMSTranslationBundle/Tests/Translation/Extractor/File/TwigFileExtractorTest.php:143
/home/albert/projects/JMSTranslationBundle/Tests/Translation/Extractor/File/TwigFileExtractorTest.php:117

@acasademont
Copy link
Contributor Author

@schmittjoh any ideas for the failing test?

@schmittjoh schmittjoh closed this Jan 24, 2013
@schmittjoh schmittjoh reopened this Jan 24, 2013
@acasademont
Copy link
Contributor Author

@schmittjoh do you need something more for this PR? Thanks!

@schmittjoh schmittjoh closed this Feb 11, 2013
@schmittjoh schmittjoh reopened this Feb 11, 2013
@schmittjoh
Copy link
Owner

Just checking whether Travis works now.

@acasademont
Copy link
Contributor Author

doesn't seem so :(

@acasademont
Copy link
Contributor Author

Seems like AopBundle is not loading property. Strange because it is in the require section of the DiExtraBundle...

@gmoreira
Copy link

@acasademont the path problem is due to the twig extensions not being loaded correctly in the unit test, i've addressed this in my pull request regarding extracting twig embeds.

@acasademont
Copy link
Contributor Author

Ah! This is great news :) Hope we can manage to finally pass the tests. As I said now there is a problem with AopBundle not being loaded

@sdepablos
Copy link

@schmittjoh could you please take a look to this and do the merge? Each time I try extract translation messages I get changes in a lot of files because of the dynamic parts of the forms not being correctly parsed. Thanks!

@tiagojsag
Copy link

+1

@schmittjoh schmittjoh merged commit 4095953 into schmittjoh:master May 18, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@acasademont @schmittjoh @gmoreira @sdepablos @tiagojsag