Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bojanz’s picture

Assigned: Unassigned » bojanz
bojanz’s picture

Status: Active » Needs review
FileSize
2.96 KB
-  $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

#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

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

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

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

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

and the patch... :-)

kim.pepper’s picture

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

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

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

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

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.