Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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!