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

bojanz’s picture

Assigned:Unassigned» bojanz
bojanz’s picture

Status:Active» Needs review
FileSize
2.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.

kim.pepper’s picture

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.

bojanz’s picture

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

bojanz’s picture

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

kim.pepper’s picture

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

kim.pepper’s picture

Status:Needs work» Needs review
FileSize
1014 bytes
2.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.

larowlan’s picture

+++ 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

kim.pepper’s picture

FileSize
2.17 KB
3.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.

kim.pepper’s picture

Status:Needs work» Needs review
FileSize
686 bytes
3.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.

sidharthap’s picture

Assigned:bojanz» sidharthap

assigning it to me.

sidharthap’s picture

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 ?

sidharthap’s picture

Status:Needs work» Needs review
FileSize
2.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.

kim.pepper’s picture

Status:Needs work» Needs review
FileSize
2.36 KB
3.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.

tim.plunkett’s picture

+++ 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.

kim.pepper’s picture

FileSize
2.79 KB

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

kim.pepper’s picture

FileSize
4.4 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View

and the patch... :-)

kim.pepper’s picture

FileSize
932 bytes
4.19 KB
FAILED: [[SimpleTest]]: [MySQL] 58,095 pass(es), 10 fail(s), and 37 exception(s). View

Forgot to remove the unused EntityControllerInterface

tim.plunkett’s picture

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

This looks prefect. RTBC if green...

kim.pepper’s picture

FileSize
1022 bytes
4.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.

kim.pepper’s picture

Status:Needs work» Needs review
FileSize
990 bytes
5.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.

kim.pepper’s picture

double-post

effulgentsia’s picture

Status:Needs review» Reviewed & tested by the community

Looks great.

effulgentsia’s picture

Component:base system» taxonomy.module
effulgentsia’s picture

Status:Reviewed & tested by the community» Needs review
FileSize
1.48 KB
4.35 KB
PASSED: [[SimpleTest]]: [MySQL] 58,335 pass(es). View
tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community

Looks good, thanks!

alexpott’s picture

Issue tags:+Needs reroll

Patch no longer applies

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
tim.plunkett’s picture

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

Only leftovers now.

alexpott’s picture

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
kim.pepper’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
FileSize
807 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.

kim.pepper’s picture

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

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

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