See #2666798-25: Add a entity revision parameter converter & route enhancer. Classic unit test fail and nobody spotted it, including myself.

CommentFileSizeAuthor
#2 entity-revision-services-2688668-2.patch987 bytesberdir

Comments

Berdir created an issue. See original summary.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review
StatusFileSize
new entity-revision-services-2688668-2.patch987 bytes

I guess we need tests for this? Should we add a revision route to EntityTestRev or so and try to access it? The entity module does that using a kernel test which was enough to catch this.

dawehner’s picture

dawehner
Primary language German
commented

Ups ...

bojanz’s picture

bojanz commented

The EntityRevisionRouteEnhancer in Entity API had a priority 20. This one has none. Not relevant?

EntityTestRev has a revision route, we can start from there for tests.

dawehner’s picture

dawehner
Primary language German
commented

I guess we really better have some form of integration test coverage.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Indeed, we have this:

entity.entity_test_rev.revision:
  path: '/entity_test_rev/{entity_test_rev}/revision/{entity_test_rev_revision}/view'
  defaults:
    _entity_view: 'entity_test_rev'
  requirements:
    _access: 'TRUE'

I can't see anyting that actually uses this and as far as I see, it's also bogus as it does a normal entity view of the ID?

So we should update that to follow the pattern that we are expecting and do a GET request to it. That should be enough?

bojanz’s picture

bojanz commented
Status: Needs work » Needs review
Issue tags: -Needs tests
StatusFileSize
new 2688668-7-fix-revision-converter-enhancer.patch4.12 KB

So, we're missing two things:
1) The service definitions.
2) A controller response for the rendered revision

Added EntityViewController::viewRevision(), fixed the test route to use it, expanded EntityViewControllerTest.
I believe this is as minimal as it gets. A bigger patch could always introduce '_entity_revision_view' to match the existing '_entity_view', but that could be a different issue too.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Thank you for adding this new test coverage!

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Thanks. Looks good to me to.

I'd say instead of adding _entity_revision_view, we could look into generating the revision view route automatically instead? Do we have an issue for that already?

bojanz’s picture

bojanz commented

@berdir
We can repurpose #2666818: [PP-2] Add a generic revision VIEW controller for it (since we have the controller, but not the route provider).

  • catch committed fff3895 on 8.2.x 
    Issue #2688668 by Berdir, bojanz: Register the new entity revision param...

  • catch committed ce7e21f on 8.1.x 
    Issue #2688668 by Berdir, bojanz: Register the new entity revision param...
catch’s picture

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

Committed/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!

  • catch committed e89076a on 8.2.x 
    Revert "Issue #2688668 by Berdir, bojanz: Register the new entity...

  • catch committed 15492d5 on 8.1.x 
    Revert "Issue #2688668 by Berdir, bojanz: Register the new entity...
catch’s picture

catch
he/him
Primary language English
commented
Status: Fixed » Needs review

PHP7 failed immediately after this commit, reverting for now - will send for DrupalCI run.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

Yes failing badly, but no idea why. Did another re-test, still fails.

alexpott’s picture

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

It's nothing to do with this issue... see #2695785: PHP7 tests with MySQL and Postgres failing

alexpott’s picture

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

Committed f23ff6a and pushed to 8.1.x and 8.2.x. Thanks!

  • alexpott committed fbb5236 on 8.2.x 
    Issue #2688668 by Berdir, bojanz: Register the new entity revision param...

  • alexpott committed f23ff6a on 8.1.x 
    Issue #2688668 by Berdir, bojanz: Register the new entity revision param...
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Fixed

Something weird is going on with issue statues...

Status: Fixed » Closed (fixed)

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