Edit broken because of #1043198 and routing system bug + missing test coverage [#1996378]

#1043198: Convert view modes to ConfigEntity converted view modes to entities. By virtue of the magic that is Drupal\Core\ParamConverter\EntityConverter, this implies that any route that uses {view_mode} will receive an upcasted view mode entity object instead of a view mode string identifier (like full or teaser).

edit.routing.yml contains

edit_field_form:
  pattern: '/edit/form/{entity_type}/{entity}/{field_name}/{langcode}/{view_mode}'

Hence upcasting is also applied to this route. But it's not expected nor needed by EditController::fieldForm(…, $view_mode). So the solution is to do a simple s/view_mode/view_mode_id/ in both the function arguments and the routing yml file.

The patch at #1043198: Convert view modes to ConfigEntity forgot to update this part. But that's forgivable, because there should have been test coverage for this. That's my fault.

However, this was extremely hard to track down because it was not EditController::fieldForm() that failed. It was the routing system itself that failed. Presumably because there's two different entities that are being upcasted in a single route: {entity} and {view_mode}. The result was an extremely unhelpful HTTP 404 status and A fatal error occurred:, without any additional information, nor a watchdog entry.
This issue is solely about fixing Edit module, not about the above problem(s) in the routing system.

The exception itself is available at https://gist.github.com/wimleers/5591825.

Comment	File	Size	Author
#6	1996378-6.patch	5.85 KB	Wim Leers
#6
#6	interdiff.txt	950 bytes	Wim Leers
#5	1996378-5.patch	5.29 KB	Wim Leers
#5
#5	interdiff.txt	790 bytes	Wim Leers
#2	1996378-2.patch	5.03 KB	Wim Leers
#2

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 16 May 2013 at 14:19

Title:

Edit broken because of #1043198, but really because of missing test coverage

» Edit broken because of #1043198 and routing system bug, but really because of missing test coverage

Comment #2

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 16 May 2013 at 14:29

Status:

Active

» Needs review

File	Size
1996378-2.patch	5.03 KB

Comment #2.0

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 16 May 2013 at 14:29

Issue summary:

View changes

Link to https://gist.github.com/wimleers/5591825.

Comment #3

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 16 May 2013 at 14:35

Title:

Edit broken because of #1043198 and routing system bug, but really because of missing test coverage

» Edit broken because of #1043198 and routing system bug + missing test coverage

Great added test coverage :)

+++ b/core/modules/edit/lib/Drupal/edit/EditController.phpundefined
+++ b/core/modules/edit/lib/Drupal/edit/EditController.phpundefined
@@ -96,7 +96,7 @@ public function metadata(Request $request) {

@@ -96,7 +96,7 @@ public function metadata(Request $request) {
    * @return \Drupal\Core\Ajax\AjaxResponse
    *   The Ajax response.
    */
-  public function fieldForm(EntityInterface $entity, $field_name, $langcode, $view_mode) {
+  public function fieldForm(EntityInterface $entity, $field_name, $langcode, $view_mode_id) {
     $response = new AjaxResponse();

phpdoc not updated!

Comment #4

fubhy CreditAttribution: fubhy commented 16 May 2013 at 14:36

Title:

Edit broken because of #1043198 and routing system bug + missing test coverage

» Edit broken because of #1043198 and routing system bug, but really because of missing test coverage

Thanks Wim, RTBC if green. This is really a problem introduced by routing/entityconverter and how it functions in general. We are trying to fix that over at #1943846: Improve ParamConverterManager and developer experience for ParamConverters

Comment #5

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 16 May 2013 at 15:00

Title:

Edit broken because of #1043198 and routing system bug, but really because of missing test coverage

» Edit broken because of #1043198 and routing system bug + missing test coverage

File	Size
interdiff.txt	790 bytes
1996378-5.patch	5.29 KB

#3: d'oh :/

#4: thanks!

Comment #6

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 16 May 2013 at 15:32

File	Size
interdiff.txt	950 bytes
1996378-6.patch	5.85 KB

D'oh… forgot to update the place where the $view_mode variable was being used :/

Comment #7

fubhy CreditAttribution: fubhy commented 16 May 2013 at 16:56

Status:

Needs review

» Reviewed & tested by the community

Comment #8

chx CreditAttribution: chx commented 16 May 2013 at 18:21

alternatively, just rip out the whole routing system. it'd be much better. (Ceterum censeo Carthaginem esse delendam)

Comment #9

Crell CreditAttribution: Crell commented 16 May 2013 at 19:24

+++ b/core/modules/edit/lib/Drupal/edit/Tests/EditLoadingTest.php
@@ -165,7 +175,51 @@ protected function retrieveMetadata($ids) {
+    // Perform HTTP request.
+    return $this->curlExec(array(
+      CURLOPT_URL => url('edit/form/' . $field_id, array('absolute' => TRUE)),
+      CURLOPT_POST => TRUE,
+      CURLOPT_POSTFIELDS => $post . $this->getAjaxPageStatePostData(),
+      CURLOPT_HTTPHEADER => array(
+        'Accept: application/json',
+        'Content-Type: application/x-www-form-urlencoded',
+      ),

Why are we using curlExec directly in Simpletest? I thought Simpletest had access to Guzzle now...

Comment #10

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 16 May 2013 at 19:58

#9: Because I'm copy/pasting from earlier code that I wrote to do this (and which was extremely painful to write BTW), in the same file. WebTestBase should have a utility method to build custom AJAX requests, but it does not.

We can convert to Guzzle later on. It's out of scope to deal with that in this issue. Also, if it were required to use Guzzle, then all existing tests that use curlExec() should have been converted.

Comment #11

Crell CreditAttribution: Crell commented 16 May 2013 at 20:58

Sigh, Simpletest.

We should convert most/all curlExec() to Guzzle, but if this is an existing pattern we don't need to do it here. No sense blocking a critical on that when it could happen at any time, as it's not an API change.