Support from Acquia helps fund testing for Drupal Acquia logo

Comments

legolasbo’s picture

Title: Remove form_set_cache() » Remove form_get_cache()
FileSize
718 bytes

Attached patch removes form_get_cache completely. Leaving this issue in the Active state untill #2355179: Remove usage of form_get_cache() and form_set_cache() gets committed to prevent a test fail.

er.pushpinderrana’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 1: remove_form_get_cache-2355187-1.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1: remove_form_get_cache-2355187-1.patch, failed testing.

skipyT’s picture

Status: Needs work » Needs review
FileSize
765 bytes
1.57 KB

we still have one form_get_cache in FormCachetest::testCacheToken, thats why the tests are failing. I updated the patch.

alexpott’s picture

Issue summary: View changes
Status: Needs review » Needs work

Can we also remove form_set_cache() too - and also add this issue to the relevant CR.

Also we should be giving a_thakur commit credit too since they worked on #2359441: Remove form_set_cache() from form.inc

ashutoshsngh’s picture

Status: Needs work » Needs review
FileSize
1.96 KB

Done changes according to #7.

rpayanm’s picture

Status: Needs review » Needs work

In core/modules/system/tests/modules/form_test/src/Controller/FormTestController.php
The docblock of storageLegacyHandler function refers to form_get_cache and form_set_cache, my question is: should modify the dockblock?

  /**
   * Emulate legacy AHAH-style ajax callback.
   *
   * Drupal 6 AHAH callbacks used to operate directly on forms retrieved using
   * form_get_cache and stored using form_set_cache after manipulation. This
   * callback helps testing whether \Drupal::formBuilder()->setCache() prevents
   * resaving of immutable forms.
   */
alexpott’s picture

@rpayanm - nice find - yep it needs updating.

rpayanm’s picture

Status: Needs work » Needs review
FileSize
1.08 KB
3.04 KB

fixed :)

a_thakur’s picture

Status: Needs review » Reviewed & tested by the community

Applied the patch. It applies cleanly. Manually reviewed the code as well. Changing to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/includes/form.inc
@@ -18,30 +18,6 @@
 use Symfony\Component\HttpFoundation\RedirectResponse;

+++ b/core/modules/system/tests/modules/form_test/src/Controller/FormTestController.php
@@ -41,9 +41,9 @@ public function twoFormInstances() {
+   * \Drupal::formBuilder()->getCache() and stored using \Drupal::formBuilder()->setCache()
+   * after manipulation. This callback helps testing whether \Drupal::formBuilder()->setCache()
+   * prevents resaving of immutable forms.

The comments need to break at 80 characters.

ashutoshsngh’s picture

Status: Needs work » Needs review
FileSize
3.04 KB

Fixed

rpayanm’s picture

Status: Needs review » Reviewed & tested by the community

@ashutoshsngh Thank you. It's better to review create a interdiff ;)

Greetings.

catch’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -@depreacted +@deprecated

This is covered by the existing change notice at https://www.drupal.org/node/2121003 and is just removing (nearly) dead code at this point, so fine from the standpoint of #2350615: [policy, no patch] What changes can be accepted during the Drupal 8 beta phase?.

Committed/pushed to 8.0.x, thanks!

rpayanm’s picture

I make git pull and not saw the commit, I'll have to do something else?

  • catch committed b1e3644 on 8.0.x
    Issue #2355187 by ashutoshsngh, rpayanm, skipyT, legolasbo: Remove...

Status: Fixed » Closed (fixed)

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