Part of #1998638: Replace almost all remaining superglobals ($_GET, $_POST, etc.) with Symfony Request object

Files that need converting are:

  • core/modules/taxonomy/lib/Drupal/taxonomy/TermFormController.php
  • core/modules/taxonomy/taxonomy.admin.inc
Files: 
CommentFileSizeAuthor
#35 1999436-taxonomy-request-35.patch807 byteskim.pepper
PASSED: [[SimpleTest]]: [MySQL] 58,004 pass(es).
[ View ]
#33 taxonomy-1999436-33.patch1.98 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,506 pass(es).
[ View ]
#29 1999436-taxonomy-request-29.patch4.35 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 58,335 pass(es).
[ View ]
#29 interdiff.txt1.48 KBeffulgentsia
#25 1999436-taxonomy-request-25.patch5.15 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 58,124 pass(es).
[ View ]
#25 interdiff.txt990 byteskim.pepper
#23 1999436-taxonomy-request-23.patch4.19 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] 57,912 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#23 interdiff.txt1022 byteskim.pepper
#21 1999436-taxonomy-request-21.patch4.19 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] 58,095 pass(es), 10 fail(s), and 37 exception(s).
[ View ]
#21 interdiff.txt932 byteskim.pepper
#20 1999436-taxonomy-request-19.patch4.4 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#19 interdiff.txt2.79 KBkim.pepper
#17 1999436-taxonomy-request-17.patch3.34 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] 58,191 pass(es), 2 fail(s), and 32 exception(s).
[ View ]
#17 interdiff.txt2.36 KBkim.pepper
#15 1999436-taxonomy-request-15.patch.patch2.79 KBsidharthap
FAILED: [[SimpleTest]]: [MySQL] 58,115 pass(es), 4 fail(s), and 4 exception(s).
[ View ]
#11 1999436-taxonomy-request-11.patch3.45 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] 56,439 pass(es), 4 fail(s), and 4 exception(s).
[ View ]
#11 interdiff.txt686 byteskim.pepper
#9 1999436-taxonomy-request-9.patch3.44 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] 56,313 pass(es), 90 fail(s), and 48 exception(s).
[ View ]
#9 interdiff.txt2.17 KBkim.pepper
#7 1999436-taxonomy-request-7.patch2.75 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 56,572 pass(es).
[ View ]
#7 interdiff.txt1014 byteskim.pepper
#2 1999436-convert-taxonomy-to-symfony-request-2.patch2.96 KBbojanz
PASSED: [[SimpleTest]]: [MySQL] 57,115 pass(es).
[ View ]

Comments

Assigned:Unassigned» bojanz

Status:Active» Needs review
StatusFileSize
new2.96 KB
PASSED: [[SimpleTest]]: [MySQL] 57,115 pass(es).
[ View ]

-  $page            = isset($_GET['page']) ? $_GET['page'] : 0;
+  $page            = Drupal::request()->query->get('page', 0);

The formatting here is wrong (we don't do the indentation of the equals sign thingy), but it's not the job of this issue to fix that.

The TermFormController changes follow what ViewsEditFormController is doing.

Status:Needs review» Needs work

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermFormController.phpundefined
@@ -198,8 +198,13 @@ public function save(array $form, array &$form_state) {
       unset($_GET['destination']);

This needs to be removed if we are doing $query->remove() above.

No, see the added code comment. Plus, this is the same todo that ViewsEditFormController has.

No, see the added code comment. Plus, this is the same todo that ViewsEditFormController has.

That sucks. It also means there are a lot of other conversions that are going to fail for the same reason.

Status:Needs work» Needs review
StatusFileSize
new1014 bytes
new2.75 KB
PASSED: [[SimpleTest]]: [MySQL] 56,572 pass(es).
[ View ]

#1668866: Replace drupal_goto() with RedirectResponse went in so removed the manual destination handling cruft. Tested manually ok.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermFormController.phpundefined
@@ -199,9 +199,9 @@ public function save(array $form, array &$form_state) {
+    $query = \Drupal::request()->query;

Request can be injected, form controllers support injection

+++ b/core/modules/taxonomy/taxonomy.admin.incundefined
@@ -278,7 +278,14 @@ function taxonomy_overview_terms($form, &$form_state, Vocabulary $vocabulary) {
+    if (Drupal::request()->query->has('page')) {

if ($page = Drupal::request()->query->has('page')) {

+++ b/core/modules/taxonomy/taxonomy.admin.incundefined
@@ -278,7 +278,14 @@ function taxonomy_overview_terms($form, &$form_state, Vocabulary $vocabulary) {
+        'page' => Drupal::request()->query->get('page'),

then you can just use $page

StatusFileSize
new2.17 KB
new3.44 KB
FAILED: [[SimpleTest]]: [MySQL] 56,313 pass(es), 90 fail(s), and 48 exception(s).
[ View ]

Thanks for the review. This includes fixes for #8.

Status:Needs review» Needs work

The last submitted patch, 1999436-taxonomy-request-9.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new686 bytes
new3.45 KB
FAILED: [[SimpleTest]]: [MySQL] 56,439 pass(es), 4 fail(s), and 4 exception(s).
[ View ]

Make the $request param have a NULL default so it still respects the buildForm() interface method.

Status:Needs review» Needs work

The last submitted patch, 1999436-taxonomy-request-11.patch, failed testing.

Assigned:bojanz» sidharthap

assigning it to me.

I tried to apply the patch mentioned on #7 and #11. Both displaying error.

error: patch failed: core/modules/taxonomy/taxonomy.admin.inc:44
error: core/modules/taxonomy/taxonomy.admin.inc: patch does not apply
is i am doing something wrong here ?

Status:Needs work» Needs review
StatusFileSize
new2.79 KB
FAILED: [[SimpleTest]]: [MySQL] 58,115 pass(es), 4 fail(s), and 4 exception(s).
[ View ]

corrected patch file.
Thanks

Status:Needs review» Needs work

The last submitted patch, 1999436-taxonomy-request-15.patch.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.36 KB
new3.34 KB
FAILED: [[SimpleTest]]: [MySQL] 58,191 pass(es), 2 fail(s), and 32 exception(s).
[ View ]

This is a reroll of the code at #11. I mistakenly thought you could pass in $request via build form.

This uses the new EntityControllerInterface to inject the request via the constructor.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermFormController.php
@@ -8,12 +8,46 @@
+  public function __construct(ModuleHandlerInterface $module_handler, Request $request) {
+    $this->request = $request;

This should be in buildForm() instead, as an optional param.

StatusFileSize
new2.79 KB

Puts the request back into buildForm() by making it a proper route rather than a page callback as per #18

StatusFileSize
new4.4 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

and the patch... :-)

StatusFileSize
new932 bytes
new4.19 KB
FAILED: [[SimpleTest]]: [MySQL] 58,095 pass(es), 10 fail(s), and 37 exception(s).
[ View ]

Forgot to remove the unused EntityControllerInterface

Status:Needs review» Reviewed & tested by the community
Issue tags:+WSCCI-conversion

This looks prefect. RTBC if green...

StatusFileSize
new1022 bytes
new4.19 KB
FAILED: [[SimpleTest]]: [MySQL] 57,912 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

I put the $request param on form() instead of buildForm().

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 1999436-taxonomy-request-23.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new990 bytes
new5.15 KB
PASSED: [[SimpleTest]]: [MySQL] 58,124 pass(es).
[ View ]

This removes a test for checking that additional request paths are not used. As we have converted to a symfony route, and this is not possible, I have removed the test.

double-post

Status:Needs review» Reviewed & tested by the community

Looks great.

Component:base system» taxonomy.module

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new1.48 KB
new4.35 KB
PASSED: [[SimpleTest]]: [MySQL] 58,335 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community

Looks good, thanks!

Issue tags:+Needs reroll

Patch no longer applies

Status:Reviewed & tested by the community» Needs work

Status:Needs work» Reviewed & tested by the community
Issue tags:-Needs reroll
StatusFileSize
new1.98 KB
PASSED: [[SimpleTest]]: [MySQL] 58,506 pass(es).
[ View ]

Only leftovers now.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Patch no longer applies

git ac https://drupal.org/files/taxonomy-1999436-33.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  2030  100  2030    0     0   2554      0 --:--:-- --:--:-- --:--:--  3311
error: core/modules/taxonomy/taxonomy.admin.inc: does not exist in index

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new807 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,004 pass(es).
[ View ]

taxonomy.admin.inc was removed in #1974492: Convert taxonomy_overview_terms to a Form Controller

Only TermFormController left now.

Status:Needs review» Reviewed & tested by the community

Assuming green, moving back to RTBC as #5 is a re-roll, and Tim was happy in #33

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

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