Updated: Comment 27
Problem/Motivation
The ajax controller code was pretty much untested so a load() call is still assumes to load multiple views.
Proposed resolution
Add a proper test coverage and use $entity_storage->load() as intended/
Remaining tasks
User interface changes
API changes
Related Issues
Original report by @RdeBoer
Glossary VIew with "Use AJAX: Yes" produces multiple errors in watchdog. Steps to reproduce are below.
8.x checkout 30 July 2013:
git clone --branch 8.x http://git.drupal.org/project/drupal.git
Enable the canned View named "Glossary". It has AJAX on by default.
Visit the View at /glossary. Output looks fine. At the top of the output we see hyperlinked letters of the alphabet. Click on one. AJAX wheel spins for a while. Nothing changes.
Errors as per attachment.
The first one that occurs (the bottom one in the attachment) is:
Warning: array_flip(): Can only flip STRING and INTEGER values! in Drupal\Core\Config\Entity\ConfigStorageController->loadMultiple() (line 125 of /Applications/drupal8/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.php).
In the advanced fieldset set "Use AJAX: No" and all is good. No errors (but no AJAX).
Comment | File | Size | Author |
---|---|---|---|
#48 | 2053519.48.patch | 688 bytes | alexpott |
#40 | vdc-2053519-40.patch | 11.1 KB | dawehner |
#40 | interdiff.txt | 745 bytes | dawehner |
#38 | vdc-2053519-38.patch | 11.29 KB | dawehner |
#38 | interdiff.txt | 717 bytes | dawehner |
Comments
Comment #0.0
RdeBoeradded error msg
Comment #1
mondrakeThis seems to be a general problem if you set 'Use AJAX: Yes' on any view.
Comment #2
mondrakeThis patch seems to fix the issue. Let's see if we can add tests.
Comment #3
mondrakeComment #4
dawehnerThis needs seriously needs a functional test. If noone comes back to it, I will do it during the week.
Comment #5
mondrakeI tried tests but I am giving up. Again, I can't find adequate drupalGetAJAX/drupalPostAJAX combinations to use to emulate AJAX in simpletest, like in #2048309: Views UI Preview - navigation is broken.
Just for reference if anyone wants to pick up, this is how far I got to in a test method in Drupal\views\Tests\Plugin\PagerTest.php:
Comment #6
dawehnerThis is from my perspective a critical bug.
This adds unit tests as well as fixes some bugs beside the main part of the issue.
Comment #7
dawehnerAdd another tag.
Comment #8
dawehnerAdded followup: #2071145: Regression: Allow to change the commands of an ajax response
Comment #10
dawehnerha.
Comment #11
larowlan%s/commando/command
Wrong indentation
%s/got/has been
Manually tested on simplytest.me as per steps to reproduce, fixes the OP (screenshot below).
Comment #12
mondrakeRelated, #2074037: Add drupalPostUrl() — drupalPost()/drupalPostAjax() are for forms only, D8 JS performs non-form HTTP requests.
Setting to 'needs work' for comments in #11.
Comment #13
mondrakeComment #14
mondrakeJust #11
Comment #15
jibranUnused statement.
Where are we using this?
Why not Drupal\Component\Utility\Json::decode()?
Comment #16
dawehnerGood points.
Comment #17
jibranIt is a critical bug with double test coverage. I am unable to find anything to object. @larowlan has also reviewed the patch. So I think it is safe to RTBC :).
Comment #19
dawehnerIt simply can't be right at the first time.
Comment #20
damiankloip CreditAttribution: damiankloip commentedWe can link this to the issue I opened for that, Not sure what it as atm though :)
Sets up
We should put the obligatory @todo here to remove when we can.
Comment #21
damiankloip CreditAttribution: damiankloip commentedOh, sorry, I didn't reload the issue! me--
Comment #22
jibranFor 1 @todo issue is #2067017: Remove usage of DRUPAL_CORE_COMPATIBILITY/VERSION constants.
Comment #23
dawehnerThere we go.
Comment #24
jibranPlease revert this.
Comment #25
dawehnerThat is just part of the interdiff...
Comment #26
damiankloip CreditAttribution: damiankloip commented// @todo Remove once drupal_render IS converted to autoloadable code.
Sorry, small. RTBC apart from that.
Comment #27
dawehnerThere we go.
Comment #28
damiankloip CreditAttribution: damiankloip commentedPerfect, didn;t actually mean to set RTBC before but +1 on my own premature RTBC.
Comment #28.0
damiankloip CreditAttribution: damiankloip commentedmention Git command
Comment #29
alexpottLets move the
$response->setView($view);
inside the ifComment #30
dawehnerMoved to the inner if.
Comment #31
catchdrupal_static() is never going to be straight converted to autoloadable code. Other code being converted to services will eventually mean it's no longer needed.
Comment #32
dawehnerWell, drupal_static is used to change the drupal_get_destination(). Do you have a better suggestion what the comment should be?
Comment #33
mondrake#30: view_ajax-2053519-29.patch queued for re-testing.
Comment #34
mondrakeSetting back to needs work for #31 and #32
Comment #35
dawehnerI still think that we can't do anything against it for now.
Comment #36
webchickBouncing back to catch.
Comment #37
catchI didn't mean getting rid of drupal_static() from here, just fixing the comment. Something like 'Remove when drupal_static() is no longer needed to change drupal_get_destination()"?
Comment #38
dawehnerOh I see. Thank you!
Comment #39
mondrake+++ b/core/modules/views/tests/Drupal/views/Tests/Controller/ViewAjaxControllerTest.php
@@ -0,0 +1,197 @@
+// @todo Remove this once the constant has been converted, see
+// https://drupal.org/node/2067017.
+if (!defined('DRUPAL_CORE_COMPATIBILITY')) {
+ define('DRUPAL_CORE_COMPATIBILITY', '8.x');
+}
+
#2067017: Remove usage of DRUPAL_CORE_COMPATIBILITY/VERSION constants was committed, shall this be changed now?
Comment #40
dawehnerWell ... the one about drupal_render() is actually true as it is.
Good catch!
Comment #41
mondrake... :)
You've been quicker than my re-read.... so my edit came too late...
Cheers
Comment #43
dawehner#40: vdc-2053519-40.patch queued for re-testing.
Comment #45
dawehner#40: vdc-2053519-40.patch queued for re-testing.
Comment #46
mondrakeAll feedback addressed, back to RTBC
Comment #47
webchickCommitted and pushed to 8.x. Thanks!
Comment #48
alexpottI'm getting a test failure with this new code.
It's happening because the replacement drupal_static in ViewAjaxControllerTest does not return a reference as it should.
Comment #49
dawehnerI remember writing that code and thought: This can't make a difference :(
Comment #50
dawehner.
Comment #51
dawehnerAdded #2086687: Add a test to ensure that notices/warnings are enabled #2086679: Enable notices on the testbot (at least for core) as followups.
Comment #52
webchickShoot, I am so sorry. :(
Looks like someone already committed this.
Comment #53
webchickComment #54.0
(not verified) CreditAttribution: commentedVDC