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.
Follow-up to #2188661: Extension System, Part II: ExtensionDiscovery
Preparation for #340723: Make modules and installation profiles only require .info.yml files
Objective
- Change all code to use
Extension::getType()
instead of$file->type
. - Change all code to use
Extension::getName()
instead of$file->name
. - Make
$file->uri
the pathname of the main extension file (.profile/.module/.theme). [temporary] - Remove public access to
$file->filename
.
Comment | File | Size | Author |
---|---|---|---|
#27 | interdiff.txt | 521 bytes | sun |
#27 | extension.uri_.27.patch | 61.62 KB | sun |
#25 | interdiff.txt | 3.49 KB | sun |
#25 | extension.uri_.25.patch | 61.61 KB | sun |
#20 | extension.uri_.20.patch | 60.2 KB | Sutharsan |
Comments
Comment #1
sunComment #3
sunFixed tests.
Comment #4
longwaveI read through the patch and all the changes look good here.
Much better!
Comment #5
alexpottextension.uri_.3.patch no longer applies.
Comment #6
sunMerge branch '8.x' into extension-uri-340723-sun
Comment #7
longwaveRerolled, plus one new change in a test added by #2187717: Remove code duplication in system_list() and ThemeHandler::rebuildThemeData()
Comment #8
longwaveHa, sorry sun :)
Comment #9
longwaveComment #10
sunRe-uploading #6, see interdiff (between #7 and #6).
Comment #11
alexpottThis looks like it is unrelated.
Considering the typehint is added to the API doc for the hook lets make all the implementations correct - Missing type hinting on $file - also need to typehint theme_page_test_system_info_alter() which is currently not in the patch.
Comment #12
sunAs these were rather minor, I hope it's OK if I move straight back to RTBC.
Comment #13
alexpottCommitted 2c51074 and pushed to 8.x. Thanks!
Comment #14
sunRegarding change notices:
Edited the existing for ExtensionDiscovery to state that people should use method names instead of public properties:
https://drupal.org/node/2198695
Added a change notice for the change to
drupal_get_filename()
:https://drupal.org/node/2212815
Overall though, once we're done with the major refactoring of the extension system, any amount of change notices won't really cut it — we will much rather need a new handbook/documentation page that explains the completely new architecture, similar to the menu/routing/request upgrade handbook page.
Comment #15
alexpottHad to revert since this break testbots :(
Committed d8c718b and pushed to 8.x.
Create PR https://github.com/drush-ops/drush/pull/509 to fix. Back to rtbc awaiting merge of PR.
Comment #16
webchickHm. Why is it that these testbot breakages are not being caught prior to these patches being committed? This is at least the second in a week or so, and in both cases the patch looked hunky-dory in the issue queue.
Comment #17
sun@webchick, the reason for that seemingly is that not all testbots are running PIFR 3.x (which uses Drush to install the initial test runner environment).
But anyway, next to the Drush PR that might unblock this issue "soon-ish", we need to resolve #2206501: Remove dependency on Drush from test reviews
To do so, I restarted work on #1808220: Remove run-tests.sh dependency on existing/installed parent site — which, coincidentally, goes hand in hand with some critically important install system clean-ups I wanted to work on anyway. So, even though the rollback of this patch (just because of Drush) really drives me nuts (the 4th major issue that gets blocked on it), I have some very exciting installer clean-ups in the pipeline, ready to rumble in a few minutes. :-)
Comment #18
sunThe base building block of that work now lives in #2213357: Use a proper kernel in early installer
Comment #19
alexpottextension.uri_.12.patch no longer applies.
Comment #20
Sutharsan CreditAttribution: Sutharsan commentedPatch #12 rerolled
Comment #22
sunThe actual status here is that this issue is postponed on either #2206501: Remove dependency on Drush from test reviews or https://github.com/drush-ops/drush/pull/509 — whichever happens first.
Comment #23
Grayside CreditAttribution: Grayside commentedLooked at https://github.com/drush-ops/drush/pull/509, it is merged.
Comment #24
sunYeah, but now the latest version of Drush needs to get deployed to all testbots first..... :-/
→ That's why I'm currently working incredibly hard on #1808220: Remove run-tests.sh dependency on existing/installed parent site
Comment #25
sunEven though #1808220: Remove run-tests.sh dependency on existing/installed parent site is semi-RTBC already, I can only assume that adjusting PIFR + deploying to testbots will take another week or so...
So in order to move forward:
Restored public Extension class properties as a temporary shim until external dependencies are resolved.
The old and new
$filename
properties have different values and meanings, so renamed the new to$_filename
. This code will be reverted back to the clean state of #12, as soon as PIFR + testbots have been fixed, so I hope we can live with this slightly ugly temporary property naming workaround for a few days/weeks.Comment #27
sunFixed bogus variable.
Comment #28
sunSame patch as the RTBC/committed before, just leaving a temporary BC shim in for Drush, so I hope it's OK if I move this back to RTBC.
Comment #29
alexpottLet's try again :)
Committed 28d0b8e and pushed to 8.x. Thanks!
Comment #31
sunBelated follow-up: #2325689: Clean up temporary Extension class workarounds