Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Assigned: Unassigned » plach
Status: Active » Needs review
FileSize
1.13 KB

Untested.

dawehner’s picture

Issue tags: +Needs tests

This itself looks perfect, but needs tests for sure.

Status: Needs review » Needs work

The last submitted patch, 1: language-entity-field-access-2404739-1.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.54 KB

Aw.

plach’s picture

Looks good, thanks. Just curious: when can it happen that we have no items?

+++ b/core/modules/language/language.module
@@ -520,11 +520,21 @@ function language_field_info_alter(&$info) {
+    $bundle = $field_definition->getTargetBundle();

I cannot imagine the language field being a bundle field :)

dawehner’s picture

Looks, good thanks. Just curious: when can it happen that we have no items?

This happens always if views tries a general access checking for fields, quote from Field.php:

  /**
   * {@inheritdoc}
   */
  public function access(AccountInterface $account) {
    $base_table = $this->get_base_table();
    $access_control_handler = $this->entityManager->getAccessControlHandler($this->definition['entity_tables'][$base_table]);
    return $access_control_handler->fieldAccess('view', $this->getFieldDefinition(), $account);
  }
plach’s picture

Ha, Views! :)

plach’s picture

Assigned: plach » Unassigned
Status: Needs review » Needs work

Setting to NW for tests.

pfrenssen’s picture

Assigned: Unassigned » pfrenssen
Issue tags: +SprintWeekend2015
pfrenssen’s picture

Assigned: pfrenssen » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
1.12 KB
2.66 KB
Berdir’s picture

Status: Needs review » Needs work

Yeah, I think we don't need more than that for tests. Maybe add add an assertion for the expected return value as well?

Based on what @plach said (the language field never being a configurable field), what I did is pretty useless. I would suggest we go back to my initial patch that I never uploaded, that simply moved all that logic inside an if ($items), and adds a comment that if we have no items, then we can't decide if it is visible or not, because we do not have enough information to figure that out?

The last submitted patch, 10: language-entity-field-access-2404739-10-test_only.patch, failed testing.

pfrenssen’s picture

Assigned: Unassigned » pfrenssen

I'll have a gander.

pfrenssen’s picture

Assigned: pfrenssen » Unassigned
Status: Needs work » Needs review
FileSize
1.14 KB
2.83 KB
2.63 KB

Something like this? I initially had a more compact version but ended up with these cascading ifs for readability. I've also expanded the documentation a bit.

Berdir’s picture

Status: Needs review » Needs work
+++ b/core/modules/language/language.module
@@ -511,11 +511,21 @@ function language_field_info_alter(&$info) {
-  $langcode_key = $items->getEntity()->getEntityType()->getKey('langcode');
-  if ($field_definition->getName() == $langcode_key) {
-    $entity = $items->getEntity();
-    $config = ContentLanguageSettings::loadByEntityTypeBundle($entity->getEntityTypeId(), $entity->bundle());
-    return AccessResult::forbiddenIf(!$config->isLanguageAlterable());
+  // Only allow access to a langcode field if the entity it is attached to is
+  // configured to have an alterable language.
+  // Without items we can not decide whether or not to allow access.
+  if ($items) {
+    // Check if we are dealing with a langcode field.
+    $entity_type = \Drupal::entityManager()->getDefinition($field_definition->getTargetEntityTypeId());
+    if ($field_definition->getName() == $entity_type->getKey('langcode')) {
+      // Fall back to neutral if we do not have a bundle.
+      $bundle = $items->getEntity()->bundle();
+      if ($bundle) {
+        // Grant access depending on whether the entity language can be altered.
+        $config = ContentLanguageSettings::loadByEntityTypeBundle($entity_type->id(), $bundle);
+        return AccessResult::forbiddenIf(!$config->isLanguageAlterable());
+      }
+    }

The conditional bundle part is no longer needed, you can use the original code instead now.

The last submitted patch, 14: language-entity-field-access-2404739-14-test_only.patch, failed testing.

pfrenssen’s picture

Sure! Rolled back to the original approach.

The last submitted patch, 17: language-entity-field-access-2404739-17-test_only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 17: language-entity-field-access-2404739-17.patch, failed testing.

Berdir’s picture

Filter by username returned the right amount.	Other	UserAdminTest.php	69	Drupal\user\Tests\UserAdminTest->testUserAdmin()

Unrelated random fail?

dawehner’s picture

Status: Needs work » Reviewed & tested by the community

@Berdir
UserAdminTest does not enable language module, so it should be entirely random.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/language/language.module
@@ -511,11 +511,18 @@ function language_field_info_alter(&$info) {
-  $langcode_key = $items->getEntity()->getEntityType()->getKey('langcode');
-  if ($field_definition->getName() == $langcode_key) {
...
+    $entity_type = \Drupal::entityManager()->getDefinition($field_definition->getTargetEntityTypeId());
+    if ($field_definition->getName() == $entity_type->getKey('langcode')) {

I think we can use the original code here instead of using \Drupal as a service locator.

pfrenssen’s picture

Certainly! By building on top of previous patches I lost affinity with the original implementation.

Status: Needs review » Needs work

The last submitted patch, 24: language-entity-field-access-2404739-24.patch, failed testing.

plach’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
2.7 KB
+++ b/core/modules/language/language.module
@@ -511,11 +511,18 @@ function language_field_info_alter(&$info) {
+    if ($field_definition->getName() == $langcode_key)) {

Thanks Pieter, a surplus parenthesis slipped into the last patch ;)

The last submitted patch, 24: language-entity-field-access-2404739-24-test_only.patch, failed testing.

Berdir’s picture

In general, I would disagree with #23, because $items is optional but in this case, we have no choice but to rely on it anyway. And it was the old implementation. RTBC +1.

  • alexpott committed 629d4cb on 8.0.x
    Issue #2404739 by pfrenssen, Berdir, plach: language_entity_field_access...
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 629d4cb and pushed to 8.0.x. Thanks!

Status: Fixed » Closed (fixed)

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