Follow up for #1896268-16: Add Views integration for translation_entity

Problem/Motivation

function getTranslatorPermissions() should be probably protected

Proposed resolution

make getTranslatorPermissions() protected, parent method, and fixing that in 4 classes

Remaining tasks

  • TBD

User interface changes

No UI changes.

API changes

Change to protected.

Original report @dawehner

Follow up for #1896268-16: Add Views integration for translation_entity (and comment 14)

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Tests/Views/TranslationLinkTest.phpundefined
@@ -0,0 +1,67 @@
+  function getTranslatorPermissions() {
should be probably protected.

@tim.plunkett

Yes, it 100% should be protected, but the parent method is not, and fixing that in 4 classes is out of scope :(
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

YesCT’s picture

Title: Copy of Add Views integration for translation_entity » make getTranslatorPermissions() protected (Follow-up Add Views integration for translation_entity)

oops forgot to update the title

tim.plunkett’s picture

Title: make getTranslatorPermissions() protected (Follow-up Add Views integration for translation_entity) » Make getTranslatorPermissions() protected
Issue tags: -VDC, -VDC-integration +Novice

Anywhere that function getTranslatorPermissions appears in code, it should be protected function getTranslatorPermissions.
This is because it is only called internally, it shouldn't be public facing.

YesCT’s picture

Issue tags: +needs initial patch

thanks tim!
I think that's complete info, so it's ready for someone to pick this up and make an initial patch.

jayboodhun’s picture

Status: Active » Needs review
FileSize
5.08 KB

I have done a search and replace and made sure all these functions are protected

plach’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, thanks.

tim.plunkett’s picture

Status: Reviewed & tested by the community » Postponed

It does look good, but it's a follow-up for #1896268: Add Views integration for translation_entity, which isn't in yet, so it will need to be rerolled for that.

plach’s picture

Status: Postponed » Needs review
YesCT’s picture

Issue tags: -needs initial patch

we got the patch back in #4

Status: Needs review » Needs work

The last submitted patch, getTranslatorPermissions_protected-1897770-4.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review
FileSize
4.35 KB

does not apply. instead of rerolling, just redid the substitutions.
not my most elegant of command lines (sed bit from http://stackoverflow.com/questions/1895819/find-and-replace-in-multiple-... )

grep -R getTranslatorPermissions * | grep -v patch | cut -d":" -f1 | sort -u | xargs sed -i .sedbak 's/function getTranslatorPermissions/protected function getTranslatorPermissions/g'
find . -name "*.sedbak" -exec rm {} \;
git diff
git diff > getTranslatorPermissions_protected-1897770-10.patch

Status: Needs review » Needs work

The last submitted patch, getTranslatorPermissions_protected-1897770-10.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review
FileSize
3.51 KB

oops one too many (one was already protected)

plach’s picture

Status: Needs review » Reviewed & tested by the community

Thanks

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Updated issue summary since the info was added by tim.