Problem/Motivation

mentioned in #2337825: Update comment references to the interface for ConfigurableLanguage class due to the ConfigurableLanguageInterface (followup)

Proposed resolution

Remaining tasks

Think.

User interface changes

No.

API changes

CommentFileSizeAuthor
#19 interdiff-14-19.txt1.75 KBDésiré
#19 2340571-language_isLocked_method-19.patch15.85 KBDésiré
#14 2340571-language_isLocked_method-14.patch15.51 KBDésiré
#10 interdiff-4-10.txt1.44 KBDésiré
#10 2340571-language_isLocked_method-10.patch15.5 KBDésiré
#4 interdiff-2-4.txt850 bytesDésiré
#4 2340571-language_isLocked_method-4.patch14.74 KBDésiré
#2 2340571-language_isLocked_method-2.patch14.71 KBDésiré

Comments

martin107’s picture

martin107 commented

For reference here are examples of classes that also extend ConfigEntityInterface and implement isLocked()

\Drupal\Core\Datetime\DateFormatInterface::isLocked()
\Drupal\system\Entity\MenuInterface::isLocked()

Désiré’s picture

Désiré commented
Assigned: Unassigned » Désiré
Status: Active » Needs review
StatusFileSize
new 2340571-language_isLocked_method-2.patch14.71 KB

A first version is attached: Both LanguageInterface, ConfigurableLanguage and Language have the new isLocked() method. The locked property is changed to protected, all usages are changed to isLocked().

yesct’s picture

yesct commented
Status: Needs review » Needs work 
  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -677,7 +677,7 @@ public function getTranslation($langcode) {
-          $translation = empty($this->languages[$this->defaultLangcode]->locked) && empty($this->languages[$langcode]->locked) ? $this->addTranslation($langcode) : $this;
+          $translation = empty($locked) && !$this->getUntranslated()->language()->isLocked() ? $this->addTranslation($langcode) : $this;

    ?? :)

Désiré’s picture

Désiré commented
StatusFileSize
new 2340571-language_isLocked_method-4.patch14.74 KB
new interdiff-2-4.txt850 bytes
Désiré’s picture

Désiré commented
Status: Needs work » Needs review
yesct’s picture

yesct commented 
+++ b/core/lib/Drupal/Core/Language/Language.php
@@ -132,6 +132,13 @@ public function isDefault() {
+  public function isLocked() {
+    return (bool) $this->locked;
+  }

do we really need to cast it at a bool?

hm.
\Drupal\Core\Datetime\DateFormatInterface::isLocked()
\Drupal\system\Entity\MenuInterface::isLocked()

both do it like that... so ok with me.

---

if green, this might be ok to go. :)

The last submitted patch, 2: 2340571-language_isLocked_method-2.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 4: 2340571-language_isLocked_method-4.patch, failed testing.

martin107’s picture

martin107 commented

Hi Désiré, thank for advancing this ....

Looking at the test failure ConfigTranslationUITest, it is a big list but at the very bottom

from LanguageAccessControlHandler line 29

Cannot access protected property Drupal\language\Entity\ConfigurableLanguage::$locked

here is the line 

        return AccessResult::allowedIf(!$entity->locked)->cacheUntilEntityChanges($entity)
          ->andIf(parent::checkAccess($entity, $operation, $langcode, $account));

The $entity in this case is a ConfigurableLanguage type and as such $entity->locked needs conversion.

I hope this helps..

Désiré’s picture

Désiré commented
StatusFileSize
new 2340571-language_isLocked_method-10.patch15.5 KB
new interdiff-4-10.txt1.44 KB
  1. The protected property access (mentioned in #9) is changed to an isLocked() call. But I think here we need to do some type check, since the checkAccess() method expects an EntityInterface which have no isLocked().
  2. The other fix in \Drupal\config_translation\Access\ConfigTranslationOverviewAccess::access checks if \Drupal\Core\Language\LanguageManagerInterface::getLanguage don't returned NULL.
Désiré’s picture

Désiré commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 10: 2340571-language_isLocked_method-10.patch, failed testing.

Désiré’s picture

Désiré commented
Title: ConfigurableLanguage needs isLocked method for the locked property. Add to interface. » LanguageInterface needs isLocked method for the locked property.
Issue summary: View changes
Désiré’s picture

Désiré commented
Status: Needs work » Needs review
StatusFileSize
new 2340571-language_isLocked_method-14.patch15.51 KB

Patch rerolled from #10.

yesct’s picture

yesct commented

looking at the interdiff in #10,
yeah, I think a type hint or something is a good idea.

Status: Needs review » Needs work

The last submitted patch, 14: 2340571-language_isLocked_method-14.patch, failed testing.

martin107’s picture

martin107 commented

Typehint is a common pattern ...

\Drupal\filter\FilterFormatAccessControlHandler.
/** @var \Drupal\filter\FilterFormatInterface $filter_format */

\Drupal\comment\CommentAccessControlHandler
/** @var \Drupal\Core\Entity\EntityInterface|\Drupal\user\EntityOwnerInterface $entity */

\Drupal\block\BlockAccessControlHandler
/** @var $entity \Drupal\block\BlockInterface */

but not consistently implemented, for another issue BlockAccessControlHandler is wrong!!

may I suggest the rarely implemented but most correct form :-

\Drupal\comment\CommentAccessControlHandler
/** @var \Drupal\Core\Entity\EntityInterface|\Drupal\user\EntityOwnerInterface $entity */

martin107’s picture

martin107 commented

Please forgive me I seem to be the source of mis-information

regarding

/** @var $variableName TYPE **/

or

/** @var TYPE $variableName **

please take #17 with a pinch of salt ( hangs head in shame )

also there is no mention of inline @var in drupal's coding standard. I think I heard some comment somewhere about it being not widely accepted.

Désiré’s picture

Désiré commented
Status: Needs work » Needs review
StatusFileSize
new 2340571-language_isLocked_method-19.patch15.85 KB
new interdiff-14-19.txt1.75 KB
  1. Type hint added to \Drupal\language\LanguageAccessControlHandler::checkAccess
  2. \Drupal\config_translation\Access\ConfigTranslationOverviewAccess::access can grant access if there is no source language
Désiré’s picture

Désiré commented
Issue tags: +Amsterdam2014
martin107’s picture

martin107 commented
Issue summary: View changes
Related issues: +#2305641: Use the "standard" format for @var inline variable type declarations

I have visually looked over the patch, everything looks good.

RTBC++

All changes appropriate, nothing out of scope.....

regarding the

/** @var $variableName TYPE **/

or

/** @var TYPE $variableName **/

thing I found a link to the issue POSTPONED says it all...

mon_franco’s picture

mon_franco commented
Assigned: Désiré » Unassigned
Status: Needs review » Reviewed & tested by the community

I have reviewed next on this patch:
- Property protected
- ConfigurableLanguage class included
- All places where isLocked should be apply are correct
- Interface isLocked is added too.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 2763695 and pushed to 8.0.x. Thanks!

  • alexpott committed 2763695 on 8.0.x 
    Issue #2340571 by Désiré | YesCT: LanguageInterface needs isLocked...

Status: Fixed » Closed (fixed)

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