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

Multilang support for content create/update #164

Merged
merged 2 commits into from Sep 13, 2018

Conversation

Projects
None yet
4 participants
@damianz5
Contributor

damianz5 commented Jun 27, 2018

Multilang support for content create/update

todo:

  • add test
  • check travis result
@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Jun 27, 2018

Codecov Report

Merging #164 into master will increase coverage by 0.21%.
The diff coverage is 98%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #164      +/-   ##
============================================
+ Coverage     67.93%   68.14%   +0.21%     
- Complexity     2314     2332      +18     
============================================
  Files           120      120              
  Lines          6121     6163      +42     
============================================
+ Hits           4158     4200      +42     
  Misses         1963     1963
Impacted Files Coverage Δ Complexity Δ
Core/Executor/ContentManager.php 92.94% <98%> (+0.83%) 140 <20> (+18) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4fd5be1...493663d. Read the comment docs.

codecov-io commented Jun 27, 2018

Codecov Report

Merging #164 into master will increase coverage by 0.21%.
The diff coverage is 98%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #164      +/-   ##
============================================
+ Coverage     67.93%   68.14%   +0.21%     
- Complexity     2314     2332      +18     
============================================
  Files           120      120              
  Lines          6121     6163      +42     
============================================
+ Hits           4158     4200      +42     
  Misses         1963     1963
Impacted Files Coverage Δ Complexity Δ
Core/Executor/ContentManager.php 92.94% <98%> (+0.83%) 140 <20> (+18) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4fd5be1...493663d. Read the comment docs.

@damianz5

This comment has been minimized.

Show comment
Hide comment
@damianz5

damianz5 Jul 11, 2018

Contributor

ping @gggeek please review

Contributor

damianz5 commented Jul 11, 2018

ping @gggeek please review

@SylvainGuittard SylvainGuittard referenced this pull request Jul 19, 2018

Merged

Added translated migration #89

1 of 1 task complete
@SylvainGuittard

This comment has been minimized.

Show comment
Hide comment
@SylvainGuittard

SylvainGuittard commented Jul 19, 2018

ping @gggeek ;)

@gggeek

This comment has been minimized.

Show comment
Hide comment
@gggeek

gggeek Jul 21, 2018

Member

Sorry for taking a while. PR reviewed, changes requested.

Member

gggeek commented Jul 21, 2018

Sorry for taking a while. PR reviewed, changes requested.

{
$languages = $this->getContentLanguages();
$languageCodes = array_combine($languages, $languages);
$hasLanguageCodesAsKeys = false;

This comment has been minimized.

@gggeek

gggeek Sep 13, 2018

Member

I'd rather do the opposite logic: start the function with True and exit with False as soon as one non-multilang field def (or multilang with wrong key) is detected.
Will merge as is, then fixup

@gggeek

gggeek Sep 13, 2018

Member

I'd rather do the opposite logic: start the function with True and exit with False as soon as one non-multilang field def (or multilang with wrong key) is detected.
Will merge as is, then fixup

*/
protected function getContentLanguages()
{
static $languages;

This comment has been minimized.

@gggeek

gggeek Sep 13, 2018

Member

I think that this (static) might not be 100% safe, as there is a contrived case where we could access many repos at the same time

@gggeek

gggeek Sep 13, 2018

Member

I think that this (static) might not be 100% safe, as there is a contrived case where we could access many repos at the same time

@gggeek gggeek merged commit 87316f8 into kaliop-uk:master Sep 13, 2018

2 checks passed

Scrutinizer 1 new issues, 6 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment