-
-
Notifications
You must be signed in to change notification settings - Fork 827
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
dev/drupal#167 - Deprecated service to be removed in Drupal 10 #21809
Conversation
(Standard links)
|
@totten @seamuslee001 how do we assess this one? |
I think this makes sense probably just needs an r-run on d9 GUI install and d7 GUI install as i think both are using civi-setup now |
test this please |
We (@BettyDolfing, @kainuk and I) are doing reviews of PR's and checked this PR. However we don't know how we could reproduce this as a user. As mentioned in the issue we would be helped with a step by step instruction on how to reproduce this. @demeritcowboy are you able to provide this? |
There are steps in the PR description where it says "one way to see it". Another way would be to set up your environment to run drupal's own unit tests but you probably aren't interested in doing that right now. Another way to sort of see it is if you look at the bottom at e.g. https://github.com/colemanw/webform_civicrm/runs/4222013159?check_suite_focus=true#step:17:482 at the bottom of the section called "Run phpunit", you'll see it lists a summary of the deprecation notices. Unfortunately it only gives a count but some of them are this one because that's how I noticed this in the first place (FYI that count of errors also includes the symphony eventdispatcher issue). |
test this please |
@demeritcowboy: Unfortunately we still don't know how to test this in CIviCRMs test environment (which is accessible at this PR). For us it requires too many technical work to easily test this. So we will leave this PR for someone else to review who knows better on how to test this. |
Ok thanks for looking. |
@eileenmcnaughton or @seamuslee001 or @totten is one of you able to review this PR? @BettyDolfing and I don't know how to review and test this. |
I've updated it to remove the fallback support for drupal 8, as per chat and to simplify. Drupal 8 has been EOL for 6 months and there's nothing civi-wise to prevent upgrading from 8 to 9. For reference this is what I had before:
|
Ok @demeritcowboy so this PR is basically dropping support for Drupal 8? Is that something we have supported up until now? I'm wondering if this needs some sort of announcement? |
See https://lab.civicrm.org/dev/drupal/-/issues/178 When I first put this up it was a no-op for everything except drupal 10, and to be honest I thought was a pretty easy PR. And then after the above ticket I decided to simplify. If there's concerns I don't see a problem with waiting until master is 5.52. |
Yay! Now these 100,000 notices will go away too: https://github.com/colemanw/webform_civicrm/runs/6812911004?check_suite_focus=true#step:18:606: |
I may have oversold it slightly - it only got rid of about 20 of those 100,000 in webform. It did get rid of all of them in civicarrot though. But most importantly it's no longer a hard block on running the installer for drupal 10. |
Overview
https://lab.civicrm.org/dev/drupal/-/issues/167
site.path
service is deprecated and will be removed in drupal 10.Before
For reasons I don't fully understand drupal uses
@trigger_error
which because of the@
hides the message and only outputs the deprecations when running in drupal's own test environment, so you can't see it doing an install (withcv core:install
). But it's there.One way to see it:
cv core:install --debug-model
which is a read-only operation that won't do anything to your install.The "site.path" service is deprecated in drupal:9.0.0 and is removed from drupal:10.0.0 blah blah...
After
Technical Details
https://www.drupal.org/node/3080612
Note the "how to be cross-compatible" was added by a contributor and it maybe makes sense in some places but I went a different route.
Comments