Editor's in-place editing AJAX endpoint broken because of #1043198 and routing system bug

Confirmed test results from #2. This fixes a pretty major bug with Edit module and is a simple find/replace of a variable name, so I'm comfortable committing from needs review. And yeah "defend against API changes" doesn't really need test coverage.

Committed and pushed to 8.x. Thanks!

This makes in-place editing broken on the full node view. Elevating to major bug as per #2037687: Can no longer in-place edit body fields. I think it would be important to have test coverage for this path to ensure it has an AJAX response (with the right kind of data?). This is not the first time we had issues in core with router handling changing. While this sounds like protecting against future API changes, when we find regressions like this, we usually add tests to ensure they don't happen again. Especially since this is not for the first time.

I disagree this would not need test coverage, but since the fix itself was committed now, we can demote to normal.

That's definitely a good point. Sorry, I did not get much sleep last night. ;P Or the past 8 weeks, hehe.

I'll try to add a test / expand the current tests inside the edit module to get coverage for this scenario.

The \Drupal\edit\Tests\EditLoadingTest class has a big testUserWithPermission() method that I could extend to do something similar as done here (asserting if the metadata for that field can be retrieved by checking if the HTTP response code is 200):

    $response = $this->retrieveMetadata(array('node/1/body/und/full'));
    $this->assertResponse(200);

I plan to check for the response of the editor URL in a similar fashion. I'll upload a patch with this idea later (after I've tested it locally).

Here's a patch that adds a couple of assertions around the existence of a route for POST /editor/{entity_type}/{entity}/{field_name}/{langcode}/{view_mode_id} by checking if a cURL POST request with AJAX headers to "editor/node/1/body/und/full" gives the expected response.
(The field is already created during the tests for other assertions as well).

I had to include the editor module in the static $modules (to enable) property of the test because that's the module that actually defines the route.

Does this provide enough coverage for this scenario or do we need more?

Can you upload a patch that reverts the committed patch, to prove that the tests do as advertised? Thanks.

Here's a patch that reverts the changes from the patch in #1, but has my extra test assertions from #8 in place.

For me locally this results in 4 fails inside EditLoadingTest.php: the 4 assertions I added. I've included a screenshot of those errors/fails:

Edit: and the patch has failed as expected, with the same 4 fails and messages. :)

I've also added an interdiff so you can easily see which lines I 'reverted' to re-create the original problem.

Awesome! Thanks for the quick turnaround. I'm not viable for RTBC-ing this myself, however.

Didn't mean to remove those tags, but "Needs tests" is now in fact not necessary anymore.

Woot, thanks for the tests! The fix looks pretty good. I think the @todo in the test makes sense as-is, it would indeed be good to test for the empty response, but currently not possible.

Yay more tests! Committed and pushed to 8.x. Thanks!