Problem/Motivation
Hello project maintainers,
This is an automated issue to help make this module compatible with Drupal 10.
To read more about this effort by the Drupal Association, please read: The project update bot is being refreshed to support Drupal 10 readiness of contributed projects
Patches will periodically be added to this issue that remove Drupal 10 deprecated API uses. To stop further patches from being posted, change the status to anything other than Active, Needs review, Needs work or Reviewed and tested by the community. Alternatively, you can remove the "ProjectUpdateBotD10" tag from the issue to stop the bot from posting updates.
The patches will be posted by the Project Update Bot official user account. This account will not receive any issue credit contributions for itself or any company.
Proposed resolution
You have a few options for how to use this issue:
- Accept automated patches until this issue is closed
If this issue is left open (status of Active, Needs review, Needs work or Reviewed and tested by the community) and the "ProjectUpdateBotD10" tag is left on this issue, new patches will be posted periodically if new deprecation fixes are needed.
As the Drupal Rector project improves and is able to fix more deprecated API uses, the patches posted here will cover more of the deprecated API uses in the module.
Patches and/or merge requests posted by others are ignored by the bot, and general human interactions in the issue do not stop the bot from posting updates, so feel free to use this issue to refine bot patches. The bot will still post new patches then if there is a change in the new generated patch compared to the patch that the bot posted last. Those changes are then up to humans to integrate.
- Leave open but stop new automated patches.
If you want to use this issue as a starting point to remove deprecated API uses but then don't want new automated patches, remove the "ProjectUpdateBotD10" tag from the issue and use it like any other issue (the status does not matter then). If you want to receive automated patches again, add back the "ProjectUpdateBotD10" tag.
- Close it and don't use it
If the maintainers of this project don't find this issue useful, they can close this issue (any status besides Active, Needs review, Needs work and Reviewed and tested by the community) and no more automated patches will be posted here.
If the issue is reopened, then new automated patches will be posted.
If you are using another issue(s) to work on Drupal 10 compatibility it would be very useful to other contributors to add those issues as "Related issues" when closing this issue.
Remaining tasks
Using the patches
- Apply the latest patch in the comments by Project Update Bot or human contributors that made it better.
- Thoroughly test the patch. These patches are automatically generated so they haven't been tested manually or automatically.
- Provide feedback about how the testing went. If you can improve the patch, post an updated patch here.
Providing feedback
If there are problems with one of the patches posted by the Project Update Bot, such as it does not correctly replace a deprecation, you can file an issue in the Drupal Rector issue queue. For other issues with the bot, for instance if the issue summary created by the bot is unclear, use the Project analysis issue queue.
Comment | File | Size | Author |
---|---|---|---|
#47 | cloudflare_2.0.0_D9_compatibility-3296796-47.patch | 649 bytes | socketwench |
#44 | D10-Readiness-Status.png | 57.92 KB | kuhikar |
#44 | cloudflare-3296796.patch | 13.7 KB | kuhikar |
Issue fork cloudflare-3296796
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
Project Update Bot CreditAttribution: Project Update Bot commentedThis is an automated patch generated by Drupal Rector. Please see the issue summary for more details.
It is important that any automated tests available are run with this patch and that you manually test this patch.
Drupal 10 Compatibility
According to the Upgrade Status module, even with this patch, this module is not yet compatible with Drupal 10.
Currently Drupal Rector, version 0.13.0, cannot fix all Drupal 10 compatibility problems.
This patch does not update the
info.yml
file for Drupal 10 compatibility.Leaving this issue open, even after committing the current patch, will allow the Project Update Bot to post additional Drupal 10 compatibility fixes as they become available in Drupal Rector.
Debug info
This patch was created using these packages:
Comment #5
balintpekkerThere is a proposal to switch CloudFlare library, which either needs to be done or the existing one needs to be updated to use Guzzle 7. The
d8-contrib-modules/cloudflarephpsdk
is usingguzzlehttp/guzzle ~6.1
hence the module won't be D10 compatible until it is resolved. The library has a pull request to allow Guzzle 7, the only thing needed is to merge it.Comment #6
Project Update Bot CreditAttribution: Project Update Bot commentedThis is an automated patch generated by Drupal Rector. Please see the issue summary for more details.
It is important that any automated tests available are run with this patch and that you manually test this patch.
Drupal 10 Compatibility
According to the Upgrade Status module, even with this patch, this module is not yet compatible with Drupal 10.
Currently Drupal Rector, version 0.13.1, cannot fix all Drupal 10 compatibility problems.
This patch does not update the
info.yml
file for Drupal 10 compatibility.Leaving this issue open, even after committing the current patch, will allow the Project Update Bot to post additional Drupal 10 compatibility fixes as they become available in Drupal Rector.
Debug info
This patch was created using these packages:
Comment #7
balintpekkerThe rector patch posted in #6 is already part of MR !7
Comment #8
nkoporecReviewed the MR, left some questions, so setting this a 'Needs work' for now.
Comment #9
balintpekkerComment #10
nkoporecThe changes looks good, marking as RTBC.
Comment #11
Project Update Bot CreditAttribution: Project Update Bot commentedComment #12
jcnventura CreditAttribution: jcnventura at 1xINTERNET commentedExcept that it doesn't really work since trying to install this on Drupal 10 will result in:
Looking into the issue queue of contrib-modules/cloudflarephpsdk, I see the following issue: Deprecate this library in favour of the official one
As such, Drupal 10 support is blocked on progress of #2915341: Proposal to switch Cloudflare library. Once the official library is being used, we can then think about adding Drupal 10 support.
Comment #13
balintpekker@jcnventura is right, please see comment #5, and PR #38 for cloudfarephpsdk: https://github.com/d8-contrib-modules/cloudflarephpsdk/pull/38
Comment #14
mglamanSDK was released https://github.com/d8-contrib-modules/cloudflarephpsdk/releases/tag/1.0.0
Comment #15
jcnventura CreditAttribution: jcnventura at 1xINTERNET commentedAs per https://www.drupal.org/node/3070687#test, test modules should be in Testing package, and skip core_version_requirement.
Comment #16
VladimirAusThanks everyone! Hiding files so the focus is on MR.
Would be great to create 2.0.x branch and release for D10 support.
Happy to help maintain the module if required.
You can test Drupal 10 version by running
and adding
before main drupal repository.
Comment #17
jcnventura CreditAttribution: jcnventura at 1xINTERNET commented@VladimirAus Use https://github.com/mglaman/composer-drupal-lenient + composer patches instead of adding custom repos when testing D10 support. It's a lot simpler :)
Comment #18
jcnventura CreditAttribution: jcnventura at 1xINTERNET commentedTaking into account #3280603: Fix tests in 8.x-1.x, I've rebased the MR, and undid all the changes from base 8.x-1.x that don't seem to be necessary for D10 support. If the tests pass, this should be a much easier change. I need to investigate when ResponseEvent and RequestEvent were made available in Symfony, as it may be that the Drupal 8 compatibility needs to be dropped.
Comment #19
VladimirAusLooking good.
Patch applies and doesn't break D10.
Cheers 🍻
Comment #20
jcnventura CreditAttribution: jcnventura at 1xINTERNET commentedWell, the tests failed in D10. The fix is trivial, however. Let me just fix that and see if we need to supplement #3280603: Fix tests in 8.x-1.x further..
Comment #21
jcnventura CreditAttribution: jcnventura at 1xINTERNET commentedOK. All tests pass in Drupal 9.5 and Drupal 10.1. I dropped support for Drupal 8, since the new ResponseEvent and RequestEvent classes that are no longer available in Symfony 6 (used by Drupal 10) were first marked deprecated in Symfony 4.3 (which Drupal 9 uses).
Comment #23
RoSk0This needs rebase now when #3280262: Support more use cases is committed.
I'm curious is we would be able to support 9 and 10 on the same branch with the changes from
symfony/http-kernel
, like https://www.drupal.org/node/3236639 .I will try to do it myself as it wouldn't be trivial with the IP restore functionality moved into middleware.
Comment #24
RoSk0Rebased and
bypassHost
property declaration on the middleware class (was breaking tests on PHP 8.2).Lets see if tests are green now.
Comment #26
mandclu CreditAttribution: mandclu as a volunteer and at Acquia commentedThe bigger issue with the change from MASTER_REQUEST to MAIN_REQUEST in Symfony's HttpKernelInterface is that now it's impossible to have a CloudFlareMiddleware that doesn't throw a fatal error in either Drupal 9.5 or 10, at least as long as it continues to implement HttpKernelInterface.
For now I've updated it to work with Drupal 10, since that is ultimately the aim of the work here. I also added an ability for the SettingsForm to gracefully handle a ResponseException from the Cloudflare SDK.
It's down to one error on my local, I'll have to do some more investigation to figure out why the testbot isn't showing any improvement.
Comment #27
jcnventura CreditAttribution: jcnventura at 1xINTERNET commentedYes, unfortunately now that #3280262: Support more use cases has been committed and made available in a released version it seems that there needs to be a version for Drupal 10 and another for Drupal 9.
Please create a 2.x branch, and move this issue over to that branch.
Comment #28
mandclu CreditAttribution: mandclu as a volunteer and at Acquia commentedAs of the last two commits, the tests pass for Drupal 10, at least in PHP 8.1. The most recent commit resolves some deprecation errors related to dynamic properties in PHP 8.2, but there are still a number thrown related to ctools, so I've proposed changes at #3346063: Dynamic properties are deprecated PHP 8.2, explicitly declare WizardFormController::$wizardFactory that will hopefully resolve the remainder.
As for moving this issue to a new major version, there's an explanation in this article about why adding major version support in a new major version of a module creates headaches for site maintainers. Does CloudFlareMiddleware need to implement HttpKernelInterface? Could we remove that from the 8.x-1.x branch and tag a release compatible with both 9.x and 10, and then later tag a 2.0 release that adds the HttpKernelInterface implementation back in, and only support Drupal 10+. Or, we could tag a new 2.0.x release without the implementation, and then later make a 2.1.x branch that adds it back in.
Comment #29
mandclu CreditAttribution: mandclu as a volunteer and at Acquia commentedRemoving the HttpKernelInterface implementation didn't work so it looks like going to a 2.0.x branch is the only path forward, even though it will be inconvenient for site owners looking to upgrade to Drupal 10. If we're ready to move forward with that approach I can create the new branch and merge in these changes once they've been tested by someone else.
Testing locally with these changes and the patched version of ctools, the PHP 8.2 tests pass as well.
Comment #30
VladimirAusHappy with it. Thanks 🍻
Comment #31
jcnventura CreditAttribution: jcnventura at 1xINTERNET commentedI'd suggest a 2.x branch instead of a 2.0.x branch, as you can easily create 2.1.0, 2.2.0 versions out of a 2.x branch, but you'll need to keep doing many 2.y.x branches if you start with the 2.0.x branch... But you guys are the maintainers, so do whatever you think is best.
Comment #36
mandclu CreditAttribution: mandclu as a volunteer and at Acquia commentedI merged this into a new 2.0.x branch and rolled a 2.0.0-alpha1 release. My intent is to soon roll an alpha2 release that will only support Drupal 10, but I thought for the sake of upgrading that it was important to have at least one release that supported both major versions, even if it will cause fatal errors in Drupal 9.
Open to opinions on how quickly to release the alpha2.
Comment #37
RoSk0Thanks for pushing this Martin.
Can you please elaborate why you choose
2.0.x
branch , rather than suggested in #312.x
? Major version branches make more sense in semver context , easier to maintain.I would really prefer to see
2.x
branch with corresponding2.x-dev
release for the latest changes.Comment #38
mandclu CreditAttribution: mandclu as a volunteer and at Acquia commentedIn my experience, feature version branches, e.g. 2.0.x are what the community intends. For example, when you create a new project, the suggested git code creates a 1.0.x branch, not 1.x. Further, in my experience a feature version dev branch displays coupled with the corresponding tagged release, as the 2.0.x-dev and 2.0.0-alpha1 display together on this project page currently.
On a more practical note, feature version branches allow for the development of new features in a separate branch while still being able to push maintenance fixes to the current stable branch. Once we get a stable 2.0.0 release, we can work on new features in a 2.1.x branch, while still being able to tag new bugfix releases e.g. 2.0.1.
Comment #39
RoSk0Thanks. All good, as long as it's a conscious choice.
Comment #40
RoSk0It looks like we are not done here - 2.0.x is still marked compatible with Drupal core 9, which is not true.
Also test are failing on PHP 8.2, but we can address that in another issue.
Comment #41
mandclu CreditAttribution: mandclu as a volunteer and at Acquia commentedIt was mentioned above that because of deeper changes in the versions of Symfony used by D9 and D10 (and this module's reliance on classes whose definitions have changed) it is impossible to have a release that supports both major versions, at least given other changes that have been merged in since this issue was started.
In the release notes for the 2.0.0-alpha1 release, it clearly indicates that the release should only be used by Drupal 9 sites that are preparing to upgrade to Drupal 10. I myself followed this process this weekend, and it worked seamlessly. Perhaps additional documentation on that front is warranted.
My plan is to tag a 2.0.0-alpha2 release that will only be compatible with Drupal 10, but having at least one release that composer thinks is compatible with both makes it easier to achieve the major version upgrade for Drupal.
Comment #42
IT-Cru@mandclu: Would it than not be better to use 2.0.x for D9 / D10 and 2.1.x for D10 only. So it will be possible to provide fixes also for D9 compatible 2.0.x release for a while instead of blocking all development here with a 2.0.0-alpha2 which only supports D10.
Comment #43
IT-CruI've tried out 2.0.0-alpha1 with D9 which produce fatal errors like mentioned in info of this release. I think a lot of people could run into this issue, when they use upgrade_status module during D10 preparation and get a non-working cloudflare release installed in their D9 websites.
Would it be possible to avoid this with a new 2.0.0-alpha2 release which only supports D10 and mark 2.0.0-alpha1 as unsupported?
@mandclu: So I think you could ignore my previous comment #42 ;)
Comment #44
kuhikar CreditAttribution: kuhikar as a volunteer and at TATA Consultancy Services for Pfizer, Inc. commentedHello Team,
Added Patch : This is compatible for the Drupal 9 (9.5.7) and Drupal 10 (10.0.8-dev).
Created Patch on https://git.drupalcode.org/project/cloudflare/-/tree/2.0.x-dev
Patch Applied on Drupal 9 and Drupal 10:
1. Drupal 9:
2. Drupal 10:
Please include this patch in the alpha-2 release
Comment #45
kuhikar CreditAttribution: kuhikar as a volunteer and at TATA Consultancy Services for Pfizer, Inc. commentedComment #46
kuhikar CreditAttribution: kuhikar as a volunteer and at TATA Consultancy Services for Pfizer, Inc. commentedComment #47
socketwench CreditAttribution: socketwench at TEN7 commentedWhile not a solution to this issue, this patch can help site maintainers who fall upon this fatal error with the 2.0.0-alpha1 version while on D9 as a stopgap.
https://www.drupal.org/files/issues/2023-07-13/cloudflare_2.0.0_D9_compa...
Comment #48
kleinmp CreditAttribution: kleinmp commentedThis patch is allowing a site I'm working on to run in both Drupal 9.5.x and 10.0.x with version 2.0 of this module. It probably does not work for Drupal 9.4 and less.I updated the handle method to match how core is using it:https://git.drupalcode.org/project/drupal/-/blob/10.0.11/core/modules/pa...[Edit: Nevermind, patch #47 is working for me for both D9 and D10.]
Comment #49
VladimirAus@socketwench do you want to raise separate ticket for it?
I'm marking this issue as fixed as it was committed 6 month ago.
You can test it on D9 and D10 by running
Maintainers, do you help with releases? Happy to help.