Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Status: Active » Needs review
FileSize
1.96 KB

This is inside test coverage, so this is guaranteed to work. Only the code style needs to be reviewed.

Wim Leers’s picture

I found even more to be removed.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

Wim Leers’s picture

Issue tags: +quickfix
Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -quickfix

The last two hunks should set #attached on $form.

cosmicdreams’s picture

From AjaxResponse->ajaxRender:

foreach (array('css', 'js') as $type) {
      // It is highly suspicious if $_POST['ajax_page_state'][$type] is empty,
      // since the base page ought to have at least one JS file and one CSS file
      // loaded. It probably indicates an error, and rather than making the page
      // reload all of the files, instead we return no new files.
      if (empty($ajax_page_state[$type])) {
        $items[$type] = array();
      }
      else {
        $function = 'drupal_add_' . $type;
        $items[$type] = $function();
        drupal_alter($type, $items[$type]);
        // @todo Inline CSS and JS items are indexed numerically. These can't be
        //   reliably diffed with array_diff_key(), since the number can change
        //   due to factors unrelated to the inline content, so for now, we
        //   strip the inline items from Ajax responses, and can add support for
        //   them when drupal_add_css() and drupal_add_js() are changed to use
        //   a hash of the inline content as the array key.
        foreach ($items[$type] as $key => $item) {
          if (is_numeric($key)) {
            unset($items[$type][$key]);
          }
        }
        // Ensure that the page doesn't reload what it already has.
        $items[$type] = array_diff_key($items[$type], $ajax_page_state[$type]);
      }
    }

This should be handled as well.

Wim Leers’s picture

#6: no, see #2073819-4: [META] Remove direct calls to drupal_add_css() — this issue is solely about removing drupal_add_css() calls that are used to add assets. Not for any others, like these, which are essentially "let's see which CSS and JS were added to this page". They're a different problem space, one that we can only fix once all calls that add assets are gone! :)

xjm’s picture

Issue tags: +Prague Hard Problems
vijaycs85’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
6.14 KB

Re-roll + addressing #5

Status: Needs review » Needs work

The last submitted patch, 10: 2095335-ajax-10.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review

Locally test is fine. So 10: 2095335-ajax-10.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 10: 2095335-ajax-10.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
714 bytes
6.13 KB

Sorry testbot, it was my fault...

Wim Leers’s picture

Assigned: Wim Leers » vijaycs85
Status: Needs review » Reviewed & tested by the community
Issue tags: +Performance, +Spark, +sprint
Parent issue: » #2073819: [META] Remove direct calls to drupal_add_css()
FileSize
6.39 KB
1.88 KB

Was going to RTBC this, but found three very small nitpicks. Fixed those. I don't think they should prevent me from RTBC'ing.

vijaycs85’s picture

Changes in #15 looks good to me, so +1 for RTBC.

Just a question:

+++ b/core/modules/system/tests/modules/ajax_forms_test/ajax_forms_test.module
@@ -569,7 +569,6 @@ function ajax_forms_test_lazy_load_form_submit($form, &$form_state) {
-    $form['#attached'] = $attached;

As per #5, addd this line. Can we re move drupal_render() and keep $form['#attached']? will that make any difference? or better way of doing?

Wim Leers’s picture

#16: I tried doing that, but that caused the tests to fail. It'd be cleaner, so I'd much prefer that, but I couldn't get it to work (I really tried), and it doesn't make sense to hold this patch up on that, which is why I'm just pressing ahead. In any case it's a step forward.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed ff4a8e9 and pushed to 8.x. Thanks!

Wim Leers’s picture

Issue tags: -sprint

Status: Fixed » Closed (fixed)

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