Problem/Motivation

#2926988: Remove RevisionableEntityBundleInterface provided by core removed an interface that dependencies are relying making updates complicated.

Proposed resolution

Bring interface back but deprecate it.

Remaining tasks

User interface changes

None

API changes

Bringing back old API.

Data model changes

None

CommentFileSizeAuthor
#7 2952495-7.patch8.07 KBalexpott
#5 2952495-5.patch1.46 KBalexpott
#2 2952495-2.patch785 bytesalexpott

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Needs review
StatusFileSize
new 2952495-2.patch785 bytes
bojanz’s picture

bojanz commented

We need to identify whether the same should be done to the removed EntityViewBuilder and/or RevisionableContentEntityForm classes.
I don't think EntityViewBuilder was widely used, but RevisionableContentEntityForm was used at least by Media.

Which breakage have you seen?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Related issues: +#2926988: Remove RevisionableEntityBundleInterface provided by core, +#2952374: Update Media Entity to use 8.5.x APIs

Good point on the form - here's the breakage - #2952374: Update Media Entity to use 8.5.x APIs

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 2952495-5.patch1.46 KB

Here's a patch that includes the form. I think we can just extend the ContentEntityForm from core now.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented 
sudo -u _www php ./core/scripts/run-tests.sh --color --non-html --url http://drupal8alt.test/  --suppress-deprecations media_entity

Drupal test run
---------------

Tests to be run:
  - Drupal\Tests\media_entity\FunctionalJavascript\MediaIefIntegrationTest
  - Drupal\Tests\media_entity\FunctionalJavascript\MediaUiJavascriptTest
  - Drupal\Tests\media_entity\FunctionalJavascript\MediaViewsWizardTest
  - Drupal\Tests\media_entity\Functional\MediaAccessTest
  - Drupal\Tests\media_entity\Functional\MediaBulkFormTest
  - Drupal\Tests\media_entity\Functional\MediaUiFunctionalTest
  - Drupal\Tests\media_entity\Kernel\BasicCreationTest
  - Drupal\Tests\media_entity\Kernel\TokensTest

Test run started:
  Tuesday, March 13, 2018 - 11:53

Test summary
------------

Drupal\Tests\media_entity\FunctionalJavascript\MediaIefInteg   1 passes
Drupal\Tests\media_entity\FunctionalJavascript\MediaUiJavasc   1 passes
Drupal\Tests\media_entity\FunctionalJavascript\MediaViewsWiz   2 passes
Drupal\Tests\media_entity\Functional\MediaAccessTest           1 passes
Drupal\Tests\media_entity\Functional\MediaBulkFormTest         1 passes
Drupal\Tests\media_entity\Functional\MediaUiFunctionalTest     0 passes             1 exceptions
FATAL Drupal\Tests\media_entity\Functional\MediaUiFunctionalTest: test runner returned a non-zero error code (2).
Drupal\Tests\media_entity\Functional\MediaUiFunctionalTest     0 passes   1 fails
Drupal\Tests\media_entity\Kernel\BasicCreationTest             2 passes
Drupal\Tests\media_entity\Kernel\TokensTest                    1 passes

Test run duration: 1 min 27 sec

So we're good apart from the same test that is failing in #2952374: Update Media Entity to use 8.5.x APIs.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 2952495-7.patch8.07 KB

Okay so to make the Media test pass we have to bring back all the form code. This will at least help people move between entity module's and core's classes.

mxh’s picture

mxh commented

Won't trigger_error blow up logs? Why not just revert the removal, and add @deprecated docblock to it?
Class removals are bad at all for modules which left alpha state.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@mxh nope that's the magic of the @ in front. But it will allow contrib to detect usage in tests once contrib supports deprecation testing as core does.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@mxh we borrowed Symfony's approach to this - see https://symfony.com/doc/current/components/phpunit_bridge.html#trigger-d...

mxh’s picture

mxh commented

I see, thanks

bojanz’s picture

bojanz commented
Title: Deprecate RevisionableEntityBundleInterface instead of removing it. » [beta2 regression] Undo class removal, deprecate them instead

Media needs to move off RevisionableContentEntityForm as soon as possible, I closed all known bug reports against the issue that removed the class, since the issues are not present in core.

  • bojanz committed eaddee6 on 8.x-1.x authored by alexpott 
    Issue #2952495 by alexpott, bojanz: [beta2 regression] Undo class...
bojanz’s picture

bojanz commented
Status: Needs review » Fixed

Committed. Thanks, Alex. Beta3 incoming.

Status: Fixed » Closed (fixed)

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

gchalker@princeton.edu’s picture

gchalker@princeton.edu commented

Hello,

Would it be possible to layout the best way to install this module if you are not using composer? Such as, best module, best process, etc.? I had to put hours into finding out that this module was causing the error because it did not specify you needed composer in the title. There should be a warning or an alert upfront. Is that something that can be added to the update theme of Drupal?

If now, I would recommend a title change to indicate that it is a composer installed module. Thank you.

G.G.

bojanz’s picture

bojanz commented

This module doesn't require Composer. Also, you're in a closer issue.