Problem/Motivation

DefaultLanguageItem::applyDefaultValue() accounts for $this->getEntity() possibly returning NULL. This can never happen, though, and in a lot of places in core we already assume that a proper entity is returned always.

This is somewhat notable because from looking at the code you could assume that entities get created with the langcode 'und' sometimes even if Language module is installed. This is not the case, however.

Steps to reproduce

-

Proposed resolution

Remove the dead code.

Remaining tasks

User interface changes

-

API changes

-

Data model changes

-

Release notes snippet

CommentFileSizeAuthor
#3 3278309-3.patch1.03 KBtstoeckler
#3 3278309-2-3-interdiff.txt493 byteststoeckler
#2 3278309-2.patch821 byteststoeckler

Issue fork drupal-3278309

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

tstoeckler created an issue. See original summary.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Active » Needs review
StatusFileSize
new 3278309-2.patch821 bytes
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
StatusFileSize
new 3278309-2-3-interdiff.txt493 bytes
new 3278309-3.patch1.03 KB

Sorry about that, this should be better.

Status: Needs review » Needs work

The last submitted patch, 3: 3278309-3.patch, failed testing. View results

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs work » Needs review

Re-testing, see #3278215: (Not so) Random test failures SettingsTrayIntegrationTest.

Status: Needs review » Needs work

The last submitted patch, 3: 3278309-3.patch, failed testing. View results

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs work » Needs review

Next try now that #3278215: (Not so) Random test failures SettingsTrayIntegrationTest has actually been fixed.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

smustgrave commented
Issue tags: +Needs Review Queue Initiative

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 200, following Review a patch or merge require as a guide.

Currently running against 10.1 tests.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Tests passed in D10.1

Looks like a simple change to clean things up.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update, +Needs tests
Related issues: +#1966436: Default *content* entity languages are not set for entities created with the API

Thanks for working on this cleanup.

This could use a little more justification for why getEntity() can never be null. I drilled down awhile into the typed data API, and it seemed like there were plenty of places it could at least theoretically be null. The part about the default value of the langcode is more interesting.

With git log -L, we can see this was introduced in #1966436: Default *content* entity languages are not set for entities created with the API which seems, apparently, to be fixing the bug that this issue says can never exist.

I think this could use test coverage proving that the existing code is dead code, either new test coverage or a failing patch for existing test coverage.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +Needs subsystem maintainer review

Probably should do this too.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented
Title: Dead code in DefaultLanguageItem::applyDefaultValue() » Remove dead code in DefaultLanguageItem::applyDefaultValue()
Issue summary: View changes
Issue tags: -Needs tests, -Needs subsystem maintainer review

\Drupal\Tests\language\Kernel\EntityDefaultLanguageTest::testEntityTranslationDefaultLanguageViaCode already tests the bug where this was introduced. I think that if that passes we can ensure this is dead code. See \Drupal\Tests\language\Kernel\EntityDefaultLanguageTest::createNode. Feel free to add back the Needs tests if you think I'm wrong.

This implements \Drupal\Core\Field\FieldItemInterface, which defines @return \Drupal\Core\Entity\FieldableEntityInterface. NULL is not a valid return type (I'm really looking forward to having return types everywhere + a higher level of phpstan in core)

\Drupal\Core\Field\FieldItemList is the other only instance I could find that uses getEntity() in applyDefaultValue, and it doesn't check for NULL.

I think those are valid enough arguments to do this safely.

penyaskito opened merge request !15164

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update

Justification makes sense to me.not sure this needs tests

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Yeah agreed #16 is a good explanation of the existing test coverage.

Committed/pushed to main, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • catch committed f1274ed0 on main 
    task: #3278309 Remove dead code in DefaultLanguageItem::...

catch closed merge request !15164

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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