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

[DsServiceBundle] add ServiceLoadDataExtesion #57

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

Kshapova
Copy link

@Kshapova Kshapova commented Feb 6, 2017

Add ServiceLoadDataExtesion and ServiceBpmLoadDataExtesion (+Extesion, .yml, LoadData service)

Copy link
Member

@marioprudhomme marioprudhomme left a comment

Choose a reason for hiding this comment

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

A few things to consider before merging.

@@ -0,0 +1,8 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Member

Choose a reason for hiding this comment

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

Omit phpstorm related files.

use Oro\Bundle\LocaleBundle\Entity\Repository\LocalizationRepository;

/**
* Class ServiceExtension
Copy link
Member

Choose a reason for hiding this comment

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

Update documentation with correct class name.

if (array_key_exists('prototype', $data)) {
$item += $data['prototype'];
}
$serviceBpm = new ServiceBpm();
Copy link
Member

Choose a reason for hiding this comment

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

Space missing after if condition.

}
$serviceBpm = new ServiceBpm();

#region TITLES
Copy link
Member

Choose a reason for hiding this comment

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

Titles, Descriptions, Buttons and Presentations block could be refactored into one common block using some kind of loop.

foreach ($data['services'] as $item) {
if (array_key_exists('prototype', $data)) {
$item += $data['prototype'];
}
Copy link
Member

Choose a reason for hiding this comment

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

Space missing after if condition.

@@ -0,0 +1,20 @@
parameters:
ds.localization.repository: Oro\Bundle\LocaleBundle\Entity\Repository\LocalizationRepository
Copy link
Member

Choose a reason for hiding this comment

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

Is this parameter sitll being used?

# tags:
# - { name: oro_migration.extension, extension_name: role }

# ds.service.migration.extension.user:
Copy link
Member

Choose a reason for hiding this comment

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

Remove unneeded comment below.

class: Ds\Bundle\ServiceBundle\Migration\Extension\ServiceExtension
arguments:
- @oro_locale.repository.localization
ds.service.bpm.migration.extension.service:
Copy link
Member

Choose a reason for hiding this comment

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

Move this config below and related files to ServiceBpmBundle folder.

@@ -0,0 +1,48 @@
services:
Copy link
Member

Choose a reason for hiding this comment

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

We have removed all Data/ORM related migrations to a Demo bundle in the platform-application repository. Also, this is a client specific migration meaning it should be moved to your client project.

@@ -0,0 +1,53 @@
services:
Copy link
Member

Choose a reason for hiding this comment

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

We have removed all Data/ORM related migrations to a Demo bundle in the platform-application repository. Also, this is a client specific migration meaning it should be moved to your client project.

Copy link
Member

@marioprudhomme marioprudhomme left a comment

Choose a reason for hiding this comment

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

More details provided.


$service = new Service();

//Get all localizations
Copy link
Member

Choose a reason for hiding this comment

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

Currently, line 54, 60, etc potentially causes warnings.

Originally, the logic was being duplicated 4 times, one for each translated fields. Ideally, the refactor should be based on the translated fields, like below. Refactoring based on the localization still leaves duplicated logic for each fields like the original issue.

Here is a sample of what I'm suggesting:

foreach ([ 'titles', 'descriptions', 'buttons', 'prensentations' ] as $field) {
   // original generic translated field logic you wrote...
 foreach ($item[$field] as $localeValue) {
 +                //If location for title exists in .yml file
 +                if ($localeValue['localization']) {
 +                    $locale = $localeValue['localization'];
 +                    $localization = $this->localizationRepository->findOneBy([ 'formattingCode' => $locale ]);
 +                    if ($localization) {
 +                        $localized = new LocalizedFallbackValue();
 +                        $localized->setText($localeValue['text']);
 +                        $localized->setLocalization($localization);
 +                        $titles->add($localized);
 +                    }
 +                }
 +            }

   $service->{'setDefault' . $field}(...); // FYI Dynamic class methods also possible in php
}

Copy link
Author

Choose a reason for hiding this comment

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

I changed loop for Titles , Buttons, Presentations and Descriptions fileds
I changed a prototype for service (services.yml) You can see in comments of ServiceExtension.php file

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

Successfully merging this pull request may close these issues.

2 participants