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.

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

fix missing locale annotation for Gedmo trait #121

Merged
merged 1 commit into from Sep 3, 2016
Merged

fix missing locale annotation for Gedmo trait #121

merged 1 commit into from Sep 3, 2016

Conversation

dmarkowicz
Copy link
Contributor

@dmarkowicz dmarkowicz commented Aug 13, 2016

I am targetting this branch, because it is a bug fix for 2.x

Changelog

### Fixed
- fix missing locale annotation for Gedmo trait

Subject

\Traits\Gedmo\PersonalTranslatableTrait should use new \Traits\Gedmo\TranslatableTrait with @Gedmo\Locale annotation (same as \Model\Gedmo\AbstractTranslatable class). This fix $entity->setLocale('XX') mechanic if entity use traits.

*
* @author Nicolas Bastien <nbastien.pro@gmail.com>
*/
trait Translatable
Copy link
Member

Choose a reason for hiding this comment

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

Please follow symfony code style and rename it to TranslateableTrait

@OskarStark
Copy link
Member

@nicolas-bastien can you please verify this PR?

@OskarStark
Copy link
Member

@soullivaneuh stuck StyleCi build

*
* @author Gediminas Morkevicius <gediminas.morkevicius@gmail.com>
*
* @link http://www.gediminasm.org
Copy link
Member

Choose a reason for hiding this comment

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

this should be removed

@OskarStark
Copy link
Member

OskarStark commented Aug 16, 2016

@dmarkowicz why do we need this file? Tests/temp/.empty

'memory' => true,
);

$config = null === $config ? $this->getMockAnnotatedConfig() : $config;
Copy link
Contributor

@greg0ire greg0ire Aug 16, 2016

Choose a reason for hiding this comment

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

What about $config ?: $this->getMockAnnotatedConfig()

@dmarkowicz
Copy link
Contributor Author

Thanks for comments. BaseTestCaseORM.php was based on https://github.com/Atlantic18/DoctrineExtensions/blob/v2.4.x/tests/Gedmo/Tool/BaseTestCaseORM.php. This is why I initially left the original credits. Current version is based on Gedmo master branch and is simpler. I included your suggestions from comments above.

@@ -12,7 +12,6 @@
namespace Sonata\TranslationBundle\Traits\Gedmo;

use Sonata\TranslationBundle\Model\Gedmo\AbstractPersonalTranslation;
use Sonata\TranslationBundle\Traits\Gedmo\Translatable;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fixup this commit with the previous commit.

@dmarkowicz
Copy link
Contributor Author

I squashed PR. @OskarStark temp folder is not needed anymore, updated according to @greg0ire comments.

@@ -21,7 +20,7 @@
*/
trait PersonalTranslatable
{
use Translatable;
use TranslatableTrait;
Copy link
Member

Choose a reason for hiding this comment

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

We should merge #124 first, who does the name refactoring @OskarStark

Copy link
Member

Choose a reason for hiding this comment

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

ok then feel free to merge #124 and ping @dmarkowicz when he should rebase on this

thank you @core23 👍

@SonataCI
Copy link
Collaborator

Could you please rebase your PR and fix merge conflicts?

@dmarkowicz
Copy link
Contributor Author

Done!

@OskarStark
Copy link
Member

@core23 please give this PR a final review after #124 is merged, thank you!

/**
* {@inheritdoc}
*/
protected function setUp()
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this empty method

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

use Doctrine\ORM\Mapping\Driver\AnnotationDriver;
use Doctrine\ORM\Tools\SchemaTool;

abstract class DoctrineOrmTestCase extends \PHPUnit_Framework_TestCase
Copy link
Member

Choose a reason for hiding this comment

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

Please add some phpDoc including a description and author.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@dmarkowicz
Copy link
Contributor Author

Is there anything else I can improve here?

@OskarStark
Copy link
Member

@core23 can you please give us a final review? thank you!

{
$evm = new EventManager();

return $evm;
Copy link
Member

Choose a reason for hiding this comment

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

OTV

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What does OTV mean?

Copy link
Member

Choose a reason for hiding this comment

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

One time variable ;)

You assigned a variable and used it only once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks :)

@core23
Copy link
Member

core23 commented Sep 3, 2016

RTM, after fixing the last comments

@dmarkowicz
Copy link
Contributor Author

I pushed the last changes.

@greg0ire greg0ire merged commit 78c214d into sonata-project:2.x Sep 3, 2016
@greg0ire
Copy link
Contributor

greg0ire commented Sep 3, 2016

Thanks @dmarkowicz !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants