dev/drupal#52 Override getUrlPath() for Drupal8 #15267
Conversation
|
(Standard links)
|
mlutfy
force-pushed
the
drupal52
branch
3 times, most recently
from
6541c24 to
6f4ab70
Compare
|
is this still needed @mlutfy ? |
|
@seamuslee001 I think it is. I will try to review again, now that I have finally upgraded to Drupal 8.8. |
|
Betty and I are reviewing PRs and we came across this one. We see the latest work has been done september 11 therefor we assume this PR can be closed as it needs work and nothing happens on it. Feel free to reopen the PR if needed. |
|
Hi Jaap and Betty, thanks for the review. However, #15268 was only a partial fix. This patch is still required. |
mlutfy
force-pushed
the
drupal52
branch
2 times, most recently
from
6f4ab70 to
ba9377d
Compare
|
I updated my branch to resolve the conflicts. Apologies for the typo in my last comment. |
|
jenkins, test this please |
|
@mlutfy the test is failing because of style warnings are you able to fix them? |
There are ~15 helpers in `CRM_Utils_System` that are just pass-throughs.
These follow a pattern of (a) declaring a docblock, (b) delegating the
function-call via `__callStatic()`, and (c) putting the code in
`CRM_Utils_System_{Base,Drupal,WordPress,etc}` using class-method
polymorphism.
The call to `if (method_exists(...))` is essentially a similar
passthrough/class-method polymorphism.
This just changes the notation to match the others. For someone auditing
the Civi<=>UF contract, this should make it easier to reason about the code.
|
test this please |
|
@mlutfy Needs rebase |
jaapjansma
added
the
needs-work-not-review-ready
|
@mlutfy a lot has changed since this + it's stale. I'm gonna close for now - please e-open as appropriate |
Overview
Aims to deprecate the use of the "q" variable in Drupal8. The "q" variable is deprecated, so accessing it throws notices/warnings.
More details: https://lab.civicrm.org/dev/drupal/issues/52
This is a companion PR for #15265
Full disclosure: I have been running a variant of this patch in production for a long time, but I have not tested this specific patch.