Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
The following files need to be converted:
- core/includes/ajax.inc
- core/includes/batch.inc
- core/includes/bootstrap.inc
- core/includes/common.inc
- core/includes/errors.inc
- core/includes/form.inc
- core/includes/install.core.inc
- core/includes/install.inc
- core/includes/language.inc
- core/includes/mail.inc
- core/includes/pager.inc
- core/includes/session.inc
- core/includes/tablesort.inc
- core/tests/bootstrap.php
Comment | File | Size | Author |
---|---|---|---|
#69 | 1998696-core-include-request-68.patch | 13.03 KB | kim.pepper |
#69 | interdiff.txt | 806 bytes | kim.pepper |
#67 | 1998696-core-include-request-67.patch | 13.19 KB | kim.pepper |
#67 | interdiff.txt | 1.85 KB | kim.pepper |
#64 | 1998696-core-include-request-64.patch | 13.09 KB | kim.pepper |
Comments
Comment #1
kmcculloch CreditAttribution: kmcculloch commentedWorking on this at DrupalCon. core/includes/bootstrap.inc runs before the \Drupal object is loaded, so I can't access $_SERVER['foo'] via \Drupal::request()->server->get('foo'). I could go ahead and load \Drupal here, but I doubt that's a good idea. Skipping bootstrap.inc for now.
Comment #2
kmcculloch CreditAttribution: kmcculloch commentedpreliminary patch, addresses ajax.inc, batch.inc and common.inc
Comment #4
kmcculloch CreditAttribution: kmcculloch commentedassigning to self
Comment #5
kmcculloch CreditAttribution: kmcculloch commentedfixed previous bugs; adding errors.inc, form.inc, install.core.inc
Comment #6
kmcculloch CreditAttribution: kmcculloch commentedmodified the rest of the files
Comment #8
kmcculloch CreditAttribution: kmcculloch commentedfixed syntax error
Comment #10
kmcculloch CreditAttribution: kmcculloch commentedslowly debuggin'
Comment #11
kmcculloch CreditAttribution: kmcculloch commentedagain
Comment #13
attiks CreditAttribution: attiks commented#10: 1998696_10.patch queued for re-testing.
Comment #15
kmcculloch CreditAttribution: kmcculloch commentedrolling a patch including only inc files that have passed local testing or that I think unlikely to cause errors: ajax, batch, errors, language, mail, pager, tablesort and tests/bootstrap.php
Comment #17
kmcculloch CreditAttribution: kmcculloch commented#15: 1998696_15.patch queued for re-testing.
Comment #19
kim.pepperTagging
Comment #20
kim.pepperThis one is a potential can-o-worms. I've included just the common.inc in this patch to see if the bot gods are pleased.
Comment #21
kim.pepperTagging
Comment #22
kim.peppertagging
Comment #24
kim.pepperThe web UI installer is working fine. Just failing from drush install.
Comment #25
Crell CreditAttribution: Crell commentedRetagging, you silly bot.
Comment #26
kim.pepperLooks like there is an issue converting POST in common.inc. The request object isn't available from the container when installing with Drush.
This patch removes that conversion.
Comment #27
Crell CreditAttribution: Crell commentedLooks good visually. Bot can object if it wants.
Comment #28
kim.pepperSorry Crell. This is only the common.inc file. We need to add the rest of the files too.
Comment #29
kim.pepper#26: 1998696-core-include-request-26.patch queued for re-testing.
Comment #31
valdo CreditAttribution: valdo commentedPatch re-rolled. The changes in drupal_goto() have been discarded as this function doesn't exist anymore.
Comment #32
kim.pepperAdded the remaining conversions.
Comment #34
valdo CreditAttribution: valdo commented#32: 1998696-core-include-request-32.patch queued for re-testing.
Comment #36
kim.pepperReverted the form.inc conversions, as they break the installer.
Comment #38
kim.pepperDug around in the tests and found some that were using $_GET causing test failures. Injecting mock requests into the container now.
Comment #39
Crell CreditAttribution: Crell commentedWhy not just replace(array('alpha'=>'beta'))? (Same for the other places this patch does this.
Otherwise this looks OK to me visually.
Comment #40
kim.pepperCrell, because we are using the current request and at least in UI testing, we have a load of cruft in the url from batch api, overlay etc, that needs to be cleared. Not sure if there is a better way of doing that.
Comment #41
kim.pepperAfter speaking with @Crell in IRC I actually understand what he means now. Removed the redundant use of
query->replace(array())
.Comment #42
Crell CreditAttribution: Crell commentedThanks, Kim!
Comment #43
YesCT CreditAttribution: YesCT commentedThis issue was RTBC and passing tests on July 1, the beginning of API freeze.
Comment #44
hass CreditAttribution: hass commentedDo you know if #1969270: 403/404 pages: drupal_get_http_header('Status') returns no status code at all will be solved by this patch?
Comment #45
kim.pepperI don't believe so. It deals with the how we are using the request, not the response.
If you want to check values in the response, have a look at the last example on https://drupal.org/node/2023537. This shows how to implement an EventSubscriber for the Response event, where you can get access to it.
Kim
Comment #46
hass CreditAttribution: hass commentedThanks for the hint, but event subscriber looks not like a possible solution for Google Analytics as I need to know the response at a specific moment and I do not need to run code when the event occurs. The event occurence is not the moment when I need the information.
Comment #47
kim.pepperOk. Probably best to move the discussion from here to your specific issue.
Comment #48
alexpottlets do these changes without creating a $request variable...
eg for the last one...
Comment #49
kim.pepperMade changes in #48.
Comment #50
Crell CreditAttribution: Crell commentedAnd back.
Comment #51
alexpottCommitted 127b54a and pushed to 8.x. Thanks!
Comment #52
tstoecklerEither the Symfony Request object is trickier than I thought, or the conversion is missing the empty() check. Even if so, this definitely needs a comment, so I'm re-opening this for "needs review" again.
Comment #53
kim.pepper@tstoeckler we return FALSE if the value does not exist, so
if ($state)
doesn't need a check for empty. See http://api.symfony.com/2.2/Symfony/Component/HttpFoundation/ParameterBag...Kim
Comment #54
tstoecklerRight, so in the case that $_POST['ajax_page_state'][$type] is not set empty($_POST['ajax_page_state'][$type]) will return TRUE (previous behavior) and $state (as defined in #52) returns FALSE. So this changes the behavior, no? Sorry, if I'm missing something obvious.
Comment #55
tstoecklerPer @alexpott in IRC what I'm trying to say is that it should be
if (!$state)
notif ($state)
.This sadly means that we're also missing test coverage for ajax_render(). The best of luck to whomever wants to tackle that beast...
Comment #56
alexpottSo I've revert the commit... we obviously have no test coverage here...
Nice catch @tstoeckler
Committed f17f9cf and pushed to 8.x.
Comment #57
alexpottNow not critical... because I reverted...
Comment #58
Crell CreditAttribution: Crell commentedWhy is ajax_render() even still a thing? That should be gone entirely by now. If not, I'm sure there's an issue somewhere to remove it. If not, there should be. :-)
Comment #59
kim.pepperFixed the logic in #52. No extra tests yet. Seeing what the bot says.
Comment #60
kim.pepperajax_render() is still called from
Drupal\Core\EventSubscriber\ViewSubscriber::onAjax()
Comment #61
kim.pepper...and
Drupal\Core\EventSubscriber\ViewSubscriber::onIframeUpload()
Comment #62
kim.pepperTagging
Comment #63
kim.pepperGiven that #1959574: Remove the deprecated Drupal 7 Ajax API is a critical, I suggest we
1) Remove the ajax.inc changes from this issue
2) Remove the requirement for ajax tests
Comment #64
kim.pepperRemoved ajax.inc as per #63.
Replaced a few instances of calling deprecated functions.
Comment #65
tstoecklerOK, that looks good. I opened #2074995: Missing test coverage in ajax_render() for the missing test coverage which might or might not be crucial, depending on the fate of ajax_render() and also the equivalent code in AjaxController (i.e. if that has proper test coverage).
Comment #66
alexpottWhy bother with assigning the $request variable?
I think we can improve the readability of the code here. Assigning the $sort variable and then checking if it's set just looks odd. How about something like this?
Comment #67
kim.pepperThanks for the feedback. This makes the changes in #66. Assume RTBC is green.
Comment #68
tim.plunkett:\
Comment #69
kim.pepperGah! Somehow added an extra character. Also missed one of the items in #66.
Comment #70
kim.pepperComment #72
tim.plunkett#69: 1998696-core-include-request-68.patch queued for re-testing.
Comment #73
alexpottCommitted c1c38a9 and pushed to 8.x. Thanks!