Problem/Motivation
We should deprecate the functions in includes/schema.inc.
Just spinning off a follow up to #2124069: Convert schema.inc to the update.update_hook_registry service (UpdateHookRegistry)
Additional goal to simplify usage of "module schema" remains from drupal_write_record()
In comment #65 it was cleared that we need to split "update registry" and management ([un]instali, serving hook_schema()
) functions
In comment #87 elaborated why drupal_get_module_schema()
is not much popular in contrib and the helper class extracted to \Drupal\Core\Extension\ModuleSchema::getTables()
static method
In comment #113 it was decided that the helper class should only be needed by tests and does not have to be a trait, so it was moved to \Drupal\TestTools\Extension\SchemaInspector::getTablesSpecification()
Proposed resolution
Deprecate the following functions from includes/schema.inc
and create a new helper class with static method
\Drupal\TestTools\Extension\SchemaInspector::getTablesSpecification()
to load install file and retrieve hook_schema()
data for tests only.
drupal_get_module_schema()
- No direct replacement is provided. Test code should use\Drupal\TestTools\Extension\SchemaInspector::getTablesSpecification()
for introspection.drupal_install_schema()
,drupal_uninstall_schema()
,_drupal_schema_initialize()
- without replacements as should be implementation details of module installer
Remaining tasks
- polish comments
- add tests (for new class)
- commit
User interface changes
None
API changes
See proposed solution.
Data model changes
None
Release notes snippet
API to manage modules schema depreacted
Comment | File | Size | Author |
---|---|---|---|
#132 | 2908886-132.patch | 19 KB | andypost |
#132 | interdiff.txt | 569 bytes | andypost |
#131 | interdiff.txt | 3.97 KB | andypost |
Comments
Comment #3
martin107 CreditAttribution: martin107 as a volunteer commentedIn relation to the parent issue
I am just splitting the change record into two so we can focus.
For the moment I have just cloned the change record and deleted reference to relevant function calls.
https://www.drupal.org/node/2970993
Comment #4
martin107 CreditAttribution: martin107 as a volunteer commentedHere is a first draft ... it will cause conflicts with the parent issue because both add parameters to the ModuleInstaller class
For review, here is a summary :-
A) I started with the big patch from the parent issue and then cut out the sections that were staying in the parent issue
B) I found a home for the functions listed in the issue summary. See the new service 'schema_installer' as defined by a new class \Drupal\Core\Extension\SchemaInstaller.
C) lots of 'plumbing in' and stitching the monster into a cohesive thing.
Still todo as it will cause problems and need a reroll ... I have left out the deprecated messages see the parent issue as I think we are still to confirm acceptable text...
Comment #5
martin107 CreditAttribution: martin107 as a volunteer commentedJust making notes for the next iteration
ModulesUninstallForm -- was only partially converted
$this->schema needs to become $this->schemaInstaller
And also
I was able to make 2 methods on SchemaInstaller private --- which is a good sign in terms of decoupling .. hiding away things that should be internal.
Comment #7
martin107 CreditAttribution: martin107 as a volunteer commentedA) Fixed errors described in #5
B) I think the new installer can be lazy loaded.
Comment #10
andypostReroll
Comment #12
andypostAdded proxyclass should fix most of failures
Added some clean-up and removed function back
Replaced remaining usage of
drupal_get_installed_schema_version()
Comment #14
andypostRebase after #2975081: UpdatePathTestBase fails to re-initialize the test site (rebuild container, clear caches) after running the database updates
- Added deprecation messages
- clean-up static usage
WIP cleaning-up usage of
drupal_get_module_schema()
not sure about namespace for the service but it looks fine now
The #2124069: Convert schema.inc to the update.update_hook_registry service (UpdateHookRegistry) using
-
Drupal\Core\Schema\SchemaData
andschema.data
service name- here
Drupal\Core\Extension\SchemaInstaller
which looks more bound to module installerComment #15
andypostPointed to separate CR dgo.to/2908886 which needs update after service polished, I don't like the static cache and a way it cleaned, most of services using
reset()
orresetCache()
as separate methodAlso third argument mostly unused and could be replaced by separate method to retrieve all schemas
Patch adds a bit of clean-up
Replaced
drupal_get_module_schema()
and made this method public, otoh it is used by tests & node.install so probably it should be in other helperComment #16
andypostand it needs tests for new service when it will be polished + legacy tests
Comment #18
andypostFix proxy & status test typo
Comment #20
andypostTrying to fix test, guess static cache of service
Comment #23
andypostSo failed test shows that core will not be able to retrieve schema for modules which are not installed
Sadly this test in simpletest module... probably makes sense to move it to core
Comment #24
andypostUpdated messages for 8.8 still makes sense to split
getInstalledVersion()
into method that returns array of all schemas and the method to return installed schemaComment #25
volegerHave a few comments here:
We can move \Drupal\Core\Extension\SchemaInstallerInterface::UNINSTALLED into @see annotation and leave just SchemaInstallerInterface::UNINSTALLED in @return annotation description. This should increase the readability of the comment.
should we mention here that $schema_installer parameter will be required in the 9.0.0 version? or will we leave that as it is?
Type hint "array" missing for $schema
Comment #26
martin107 CreditAttribution: martin107 as a volunteer commented@voleger - thanks for the review.
I am working on this now.. As I start all the points look reasonable.
Comment #27
martin107 CreditAttribution: martin107 as a volunteer commentedWhen it came to sorting out #25.2
When I remove the @deprecated tags then the stroke through is removed on my IDE - which I like.
So in the end I just adopted the shortname convention when referring the classes/methods
Comment #28
volegerThis part looks good
But we have Needs tests tag, so at least we need to add legacy tests for those functions.
Comment #29
volegerComment #30
kim.pepperAdded legacy tests for the deprecated functions, and updated the change record.
I found a major WTF with getInstalledVersion() which takes a flag to return an array of all modules?? Why aren't we just using drupal_get_schema_versions() or adding a new method to return all installed schema versions?
Comment #31
kim.pepperDiscussed this issue with @claudiu.cristea at DrupalCon Seattle and agreed rather than getInstalledVersion($module, $reset = FALSE, $array = FALSE) we can split this into three methods:
I think this makes a cleaner API.
Still need to update deprecation message and CR but thought I'd post a patch here for review.
Comment #33
kim.pepperSeems the above changes didn't take into account initializing the static cache. I've added a protected method to do that.
Comment #35
kim.pepperMissed a call to resetCache() in UpdateScriptTest
Comment #36
andypostNaming probably better to make inline with #2124069: Convert schema.inc to the update.update_hook_registry service (UpdateHookRegistry)
So kinda database.schema.installer
Comment #37
daffie CreditAttribution: daffie commentedComment #38
daffie CreditAttribution: daffie commentedA quick review of the current patch:
Maybe a stupid question, but can we not use the operator ?: here?
Why are we doing this. When we are removing the descriptions from the schema to improve serialize() and unserialize()?
This change looks to me as an API change.
Not sure if this kind of testing is allowed?
Can we remove the variable $schema.
Missing 2 parameters in the docblock.
The return value type looks wrong to me.
I am not sure if this is what the non-core/contrib developer expects.
Comment #39
vacho CreditAttribution: vacho at Skilld commentedPatch #35 rerolled
Comment #40
andypostHere's a re-roll + fixes for #38 and refactored deprecation tests to not fail
1) no way (tested on php 7.3) - it reports
Undefined index: color
for deprecation test2) no idea why it supposed to work this way but conversion should not change logic (actually this method is never used in core with TRUE as last argument in `install()/uninstall()` of new service)
3) that's strange case - not installed module should not create tables and result should be empty array, no idea how it passed before
4) fixed
5) can't because this array passed by reference (but the test factored)
6) this 2 parameters not needed in new interface
7) it should return string or array, but int is valid as well because we don't do strict type in setter
8) yes, it means that schema should be altered only on read
Comment #43
daffie CreditAttribution: daffie commentedThe patch needs a reroll and the deprecations need to be for 9.1.
Comment #44
Suresh Prabhu Parkala CreditAttribution: Suresh Prabhu Parkala at Specbee commentedComment #45
Suresh Prabhu Parkala CreditAttribution: Suresh Prabhu Parkala at Specbee commentedComment #46
aleevasThe latest patch was re-rolled for 9.1.x
Comment #48
andypostFix re-roll
- deprecation test been lost
- version string changed to 9.1=>10.0
- meaningful BC was broken
Comment #49
andypostExtend test coverage for
drupal_set_installed_schema_version()
Comment #52
andypostFix all remaining usage
this places probably need BC shim but not sure
Comment #54
andypostFixed last failure and removed useless arguments
Comment #55
andypostA bit more clean-up
Comment #56
andypostFix proxy class with changes #54 - now it's ready for review
PS: form and controller from #52 is a question still
Comment #57
longwaveShould be
@trigger_error()
and__construct()
, needs updating to Drupal 9.1.0/10.0.0 and do we need to use the "drupal:9.1.0" format?It might also be better to say "The schema_installer service must be passed to ModuleInstaller::__construct()" rather than the variable name.
I think we should add BC layer here.
and here
Comment #58
andypostFixed #57
Looks there's no standard for this BC so this approach looks the most verbose
Comment #59
daffie CreditAttribution: daffie commentedPatch looks good.
When does this method return all modules?
Are these lines necessary for the resetCache() method? Can they be removed?
Why make this a private method instead of a protected method?
Why are those 3 class variables added. They are not used.
Can this line be removed. The method SchemaInstaller::setInstalledVersion() already does a resetCache().
The empty line should not be moved.
Drupal\KernelTests\KernelTestBase::installSchema()
a reference to "drupal_get_module_schema()" in a comment.Comment #60
andypostFix for #59
Meantime while fixing (5) I found that after running upgrades schema needs re-init and only in
\Drupal\Tests\system\Functional\UpdateSystem\UpdateScriptTest::testSuccessfulUpdateFunctionality()
So I changed
\Drupal\Core\Extension\SchemaInstallerInterface::resetCache()
to allow chaining which is used now.Also renamed
initCurrentVersions()
toensureInitialized()
as core using mostly this namingComment #61
andypostFix leftover mention
Comment #62
daffie CreditAttribution: daffie commentedUpdated the CR and the IS.
Comment #63
daffie CreditAttribution: daffie commentedAll my points are addressed.
All code changes look good to me.
For me the patch is RTBC.
Comment #64
martin107 CreditAttribution: martin107 as a volunteer commentedandypost++
thanks for working on this.
Comment #65
catchThanks for working on this, it would be good to tidy this up. However, I'm not sure about some aspects of the current patch.
Currently, the patch is dealing with (IMO) two things that can and probably should be separate.
1.
These are all based around
hook_update_N()
, and they apply to modules which may not use hook_schema() at all, but still rely on hook_update_N() (which is an increasing number of modules due to the entity and configuration systems).2.
These three methods all deal with hook_schema(), installing it when a module is installed, uninstalling it when the module is uninstalled. They are only ever called from ModuleInstaller, so they could potentially be moved to protected methods on that class (with the procedural functions being deprecated with no replacement).
The methods to deal with updates could then stay in the SchemaInstaller service (edit: although, probably renamed) (which would then still need to be used in both the update and module install system, but would be smaller).
Other notes from reviewing:
Could we add a protected method to get current versions, which handles the ::ensureInitialized() logic too?
We've been trying to move away from ::resetCache() methods, for example in #3047289: Standardize how we implement in-memory caches. It might be worth exploring that here, but if not we should at least open a follow-up I think.
Why does this have to call ::ensureInitialized()?
Comment #66
martin107 CreditAttribution: martin107 as a volunteer commentedIn particular 65.2 is a really good observation...
I will work on the response to 65
Comment #67
martin107 CreditAttribution: martin107 as a volunteer commentedFirst a reroll.
Comment #68
martin107 CreditAttribution: martin107 as a volunteer commentedI have made a few questionable choices, so best talk about it here.
1) I was not able to make ModuleInstaller::getSchema() private as I wanted
the reason module_test.install make use of it.
Maybe in the morning I will see an elegant way to adjust
2) The other thing I am less than happy with is the fact that ModuleInstaller has increased in size by over 100 lines
I have lived with coding standards that rules that files should be no more than 300 lines long...
and found it useful code smell that something has gotten too complex.
ModuleInstaller is now 750 lines long .. but on balance I could not find a good way of creating a ModuleHandlerSchemaHelperTrait.
so I think 750 lines is OK.
Anyway I am just posting early
Comment #69
catchmodule_test_install() could just call module_test_schema() directly, although it seems to be going through all that work just to get the string
'varchar'
, in which case I have no idea what it's doing....Since it looked a bit weird, I did some archaeology:
Was originally added here, in 2010, and used to look like this:
#620298: Schema not available in hook_install()
It was added for this test:
drupal_write_record() no longer exists, so the insert is completely unnecessary, I think we can remove it.
A few things from looking at the patch, trying not to be too nitpicky since I know it's a work in progress:
We probably need to rename this, although I don't have great ideas. Maybe SchemaVersionUpdater?
These will need updating to say there is no replacement.
We can probably remove support for $remove_descriptions in the new method.
$this->cacheBackend() is unused?
This seems unnecessary on a new class? Also unused.
If this replaced the value in $this->currentVersions(), then we wouldn't need ::resetCache() at all afaict.
Comment #71
martin107 CreditAttribution: martin107 as a volunteer commentedI am postponing for a short while...
I want to add modern type checking to the new services.
Trouble comes when I want to specify a return type of 'void' to a method that will be processed by the proxy builder.
at the moment the template code insists on always returning something
I have spun off the side issue above to fix the proxybuilder.
The patch here is just where I got to when I stopped .. it is not worth a review... I am posting a memory dump.
Regarding 69... thanks for the review .. it is always appreciated.
#1 I went with SchameVersionHandler.
I dropped the Updater .. as I don't want to frighten consumers of the service, who only want to read access to the current state of things.
I appreciated the criticism of the 'Handler' suffix -- in that "it will only encourage future coders to use it as a dumping ground" .. but I think the "SchemaVersion" part is already very specific so that concern wont arise.
#2 not yet fixed.
#3 I agree fixed.
#4 good much cleaner
#5 my goof ... thanks for the fix.
#6 work in progress.
In short postponed "Not worth running tests"... yet
Comment #72
andypostThis is not considered yet, also voids could be added in follow up
Comment #73
andypostFix #70.6 and make it work
About naming - it looks more like "schema_registry" as it stores hook_update numbers
I still think about separation from #65 - in new patches some duplicated code exists (even after clean-up)
Comment #75
andypostFix tests
Comment #77
daffie CreditAttribution: daffie commentedOnly trying to fix the tests.
Comment #78
martin107 CreditAttribution: martin107 as a volunteer commentedOn reflection it was strange to "down tools" so abruptly and work on the side issue.
daffie and andypost thanks for chasing down the bugs in the mess I left behind, as my focus changed.
DrupalCommunity++
I will have more time this weekend to work on this .. but I want to record a conversion with andypost on slack
A) He advances the idea that we should simplify the task .. split this issue into two
1) The module installer refactor
2) The schema registry
I think this is a good idea.
B) He suggests we have a redesign of the API
In particular the needs of contrib and test can we better served by a rethink.
I will have more time to think about this on the weekend .. but what I see now is that test makes extensive use of ::getSchema()
in a way could be redesigned
To repeat andypost concerns from slack
we need a good mental model outlined for getter/setters and when and when not to cache or reset
Anyway I hope this captures the conversation
.... more later.
Comment #79
andypost@martin107 Thank you a lot posting here!
Actually the issues already filed (listed in referenced by block)
- #2124069: Convert schema.inc to the update.update_hook_registry service (UpdateHookRegistry) - could be changed to address module installer part, will care about creating/deleting tables
- #3012523: Convert the update.inc file functions to a class - should use split as well (single class with static methods looks weird), so could use schema_registry
- #3130037: system.schema information gets out of sync with module list - to keep in mind to tackle caching
Comment #80
andypostFound bug while digging versions for updates #3143713: drupal_get_schema_versions() should return integer versions
Comment #81
martin107 CreditAttribution: martin107 as a volunteer commentedThanks for pointing out the other issues... I will help out on those where I can.
When I look over everything ... there is one aspect of the patch from 77 that is not addressed elsewhere afaics.
We have too many issues in this area so I just trying an experiment here.
and that is the encapsulation of drupal_install_schema()/drupal_uninstall_schema() and _drupal_schema_initialize() inside the ModuleInstaller
LocallyI have tested this patch on a few tests like
./vendor/bin/phpunit -c core/ ./core/tests/Drupal/KernelTests/Core/Extension/ModuleInstallerTest.php
but I want to see what testbot says.
Comment #82
andypostHere's iteration on this idea
- renamed methods:
getSchemaTables()
,installSchema()
,uninstallSchema()
- deprecated functions and added back test from previous patch
EDIT we can't make this functions as "no-op" to keep BC
Comment #83
andypostI went ahead and deprecated
drupal_get_module_schema()
hereWe still need this API - and schema should be always populated because it used in contrib http://grep.xnddx.ru/search?text=drupal_get_module_schema
Re-title to point split difference with "update registry schema" in #2124069: Convert schema.inc to the update.update_hook_registry service (UpdateHookRegistry)
I think we should add unit test for the method (meantime extended functional test as there's no unit one yet for module installer)
Comment #84
dwwMostly looks good. Obviously nice to remove more from includes. ;)
Some nits and some concerns of substance:
I believe this needs to be fully qualified class name.
Seems a bit weird to load the include not in the function that cares about the hook_schema() implementation. Shouldn't this loadInclude() happen directly in populateSchemaTables() before we try to invoke()?
I should read the whole issue thread, but are we sure we want private for these? ;)
I know core committers tend to push back on
array
for phpdocs. Maybe this is the best we can do here, since it's a deeply nested array of schema definition. But might be worth surveying other parts of core that deal with this to see if we can do better than this.::install()
isn't a method.A) It's installSchema().
B) We don't have resolution on #2341405: Decide on standard for referencing namespaced classes nor #3112830: [policy, no patch] Allow static::methodName() and/or self::methodName() in @see comments when referring to the same class but I think
self::installSchema()
is better than a raw::
version.Ditto for
::uninstall()
Looking at the rest of the patch, I'm not seeing where hook_schema_alter() is happening. ;) I'd need to look more closely at the related code. But this seems like it'd be good to have clear comments on.
This is auto-generated, right? That's why it doesn't follow our code style?
Is there anything else we can assert for here to make this test coverage more thorough?
Comment not wrapping to 80 chars.
Comment #85
andypostThank you! btw
hook_schema_alter()
is gone from d8)Feedback to #84 - fixed 1 and 6, thank you for review
1) fixed
2) is tricky - public API should ensure that .install is loaded but "private" methods should has already install files loaded, looks it needs better docs
3) TBD - technically this methods are private but as no agreement we can make them protected with @internal docs
4) Actual docs in hook_schema() that's why I just referenced it
5)
[un]install()
are the service methods which this comment refering to (newly added[un]installSchema()
are private)6) Removed as no hook in D8
7) yes, it's generated
8) that's what populate doing, gonna add unit test to set flow into stone when API is ok
9) I'm using to keep changes smaller, this comment may need rework depending on 2)
PS: NW for IS and CR
Comment #86
dwwThanks for quickly addressing all that!
1. 🙏
2.
Hrm, I'm not sure why you say this. Seems like loading the .inc file is an implementation detail that the "private" methods care about. If they're invoking that hook to do their work, I'd think it was their responsibility to do whatever it takes to do their job. I don't understand why the implementation of the public method can/should care about this. I don't know of any principle of OO or computing in general that says public methods need to load the code required by their private helpers. Would you be willing to explain your thoughts in more detail so I can better understand your reasoning? Thanks.
3. 👍
4. 👍
5. Oh! Not in the patch, the method already in
ModuleInstallerInterface
. 🤦♂️ Sorry about that, and thanks for clarifying. I still think we wantself
orstatic
here, not just::
, but maybe it doesn't matter.6. 👍
7. 👍
8. Sounds good.
9. Cool, so NW for later, once the docs are finalized. Also sounds good.
Thanks again,
-Derek
Comment #87
andyposttrying to elaborate 2)
1) It's debatable about need for this API at all, but I think it thing of past (when we used
drupal_write_record()
)2) what the Schema API is used for in contrib - to get the list of defined tables by
hook_schema()
(besides of tracking its update numbers) and I bet some custom code needs access to fields (definitions and database schema)3) install/uninstall of schema inside of module installer is implementation details (that's why moved to separate methods, private/protected TBD)
4) the new public method should not widely used (tests and custom from 2) - maybe move it to separate interface, because it looks like "schema definition introspection in installer" (interdiff in #83 shows less then 10 usages in core)
5) if we introduce this method in context of module installer it should load .install file (because called from contrib/custom) and re-use internal "data gathering" from hook (which may return NULL:) and massaging with module table name each definition .... looks like unit test))
Comment #88
andypostA bit of clean-up/polishing also making populate method protected and clarify usage for #87
Comment #89
andypostI came to static class helper for that purpose and no more API changes except deprecation
Checking the usage it useful to return all tables from schema
Comment #90
andypostMissed new file in previous patch
Comment #92
andypostFix tests
Comment #93
andypostSummary updated
Comment #94
andypostCR updated https://www.drupal.org/node/2970993
Comment #95
andypostRelated issue using
getSpecification()
as new method name so ++ to rename itComment #96
catchApart from the deprecated functions themselves, this is now only used in tests. Could it be a helper method on a test trait instead of a single method class in the core namespace?
This might mean we need to leave the existing logic in the deprecated functions until we remove them since there wouldn't be anything to call, but it would be less API surface. If a contrib module really needed the same functionality, they could copy the eight lines of code here - but that is something only a schema introspection module would need.
Comment #97
andypostMoved to
\Drupal\KernelTests\Core\Extension\ModuleSchemaTrait::getTablesSpecification()
I think this namespace better shows its dependency on module installer subsystem
Reverted old implementation ++ #96 thanks for idea (protected method is best choice, maybe is could be static but...self:: vs static::)
Also addressed #84.9 - feel free to help rewording
updated IS with new trait
Comment #99
andypostre-queued, unrelated
Drupal\Tests\quickedit\FunctionalJavascript\QuickEditFileTest
Comment #100
andypostLooks
name
is not needed at all, alsomodule
what for?Why this "massaging" is needed? Looks it also remains on
drupal_write_record()
or early views.Can't grep deeper as it was created in #1180112: Move drupal_*_schema_*() functions into one place
In https://git.drupalcode.org/project/drupal/-/commit/f5032a6cffd17604f9412...
Comment #101
andypostMaybe we can skip this injection for protected method as #2124069: Convert schema.inc to the update.update_hook_registry service (UpdateHookRegistry) will need to add one more dependency?
Comment #102
catchRegarding #100, it was added in the original 2007 commit, found via
.
3cafffe63f70f
is the commit.Given we're moving the logic to protected methods we could go ahead and remove the massaging here.
Comment #103
andypostAs I see from #144765-11: Schema API 1 the "module" key was added to be used for hook_schema_alter() to identify the the module, "name" used only to drop table (probably protection if table name changed by alter
As we have no schema alter and dwr() - it's ok to remove them, also -1 function call
Comment #104
catchThis looks great to me now.
Comment #105
kim.pepperReviewed the patch in detail and it looks good to me.
Comment #106
martin107 CreditAttribution: martin107 as a volunteer commentedComment #107
alexpottAre we sure that no db implementation relies on the additional info that's added here?
We need be careful about injecting services into this class. Ideally we'd introduce an event so we can decouple the dependencies. But the event dispatcher has issues here too - there is an issue to fix this. But if we want to inject this service then we need to refresh it from the container in \Drupal\Core\Extension\ModuleInstaller::updateKernel().
KernelTestBase already uses this trait so I don't think this is necessary.
We could just hardcode the default value. It would in some ways be a better test.
Comment #108
andypostFix for #107 (2 and 3)
1) yes not used - it just a remains of
hook_schema_alter()
(history in #100-103)4) not sure changing a test makes sense, maybe follow-up?
Comment #109
andypostI think it needs post update hook to rebuild container, otoh as code has BC it should not be required
Comment #110
catchThe container is keyed to the Drupal core version by default, so any patch release always rebuilds the container with no need for an explicit update.
This needs to be $this->connection doesn't it?
Comment #111
andypost@catch Thank you! fixed c/p error(
Comment #112
alexpott@andypost re #107.1 and the answer in #108 - sure it's not used by any core db driver implementation but there are contrib drivers and they might have used these keys. At the very least this needs explaining on the change record.
Re #107.4
You are already changing the test. There's no need for the schema here - we can hardcode the expectation and be done.
Comment #113
alexpottI'm not 100% convinced about the need for the trait. Here's why:
But thinking some more about this...
This is in the wrong location. But it also does not need to be a trait. It can be a public static on a SchemaInspectionHelper in core/tests/Drupal/TestTools - then we don't add new a method to ever KTB and you can get the sharability accross KTB and BTB (and all test types).
Comment #114
catch+1 from me.
Comment #115
andypost@alexpott thanks for clarification!
I went with
Drupal\TestTools\Extension\SchemaInspector
because- there's similar "helpers" (Comparator, ClassWriter) around so Helper is too broad
- schema is only about module extension, but "module" is too narrow
Comment #117
andypostreroll for 9.2.x after #3193955: Swap assertEqual arguments in preparation to replace with assertEquals
Comment #118
longwaveAll deprecations referring to 9.1.0 need to be bumped to 9.2.0. Otherwise this looks ready for commit.
Comment #119
andypost@longwave thank you, fixed
Comment #120
andypostLooks CR also needs update because issue is split in 2 but not sure how to deal with it
Comment #121
longwaveDraft CR exists for the other issue: https://www.drupal.org/node/2444417
So I think we just remove drupal_get_module_schema() from this CR?
Comment #122
andypostProbably better to split CRs too, so new one for this issue will mention existing when it's done
Comment #125
andypostFixed deprecation tests
Comment #126
daffie CreditAttribution: daffie commentedRerolled, because #2795567: Use Symfony's VarDumper for easier test debugging with dump() has landed.
Comment #127
daffie CreditAttribution: daffie commentedPatch looks good. Just some minor stuff.
Maybe better is to change "Database connection schema" to "Database schema service".
Why do have the parameter $schema? When have the class variable $this->connection. Is it not easier to do:
Why do have the parameter $schema? When have the class variable $this->connection. Is it not easier to do:
I think we should use the new SchemaInspector::getTablesSpecification() to get the value "Undefined".
We are adding this testtool new class and method to core. Could we document that in he IS and the CR.
Comment #128
daffie CreditAttribution: daffie commentedFor #127.2 and #172.3: Create the variable $schema first like:
$schema = $this->connection->schema();
instead or reloading the schema service every time.Comment #129
longwaveUpdated the CR to describe the new static helper method: https://www.drupal.org/node/2970993
One question I spotted while reading the patch to update the CR:
Should this use
$this->container->get('module_handler')
instead of the \Drupal wrapper?Comment #130
longwaveUpdated IS.
@daffie Re #127.4 see opposite opinion in #107.4/#112
Comment #131
andypostin functional test should not
Here's fix for #127 (2 and 3) ++ to #128
re #127.1 because schema is not service but "bound to connection"
Patch also adds type-hints to new methods
Comment #132
andypostfix pre-checks
Comment #133
longwaveAll review points addressed, this looks ready to go.
Comment #135
catchNearly eight years since #2124069: Convert schema.inc to the update.update_hook_registry service (UpdateHookRegistry) was opened, good that we were able to clean up ModuleInstaller to be more self-contained with this one.
Committed 535fb84 and pushed to 9.2.x. Thanks!
Comment #137
TR CreditAttribution: TR commentedThe issue summary says:
Please see #2820696-13: Allow schema inspection for all tables created with a valid schema. The functionality provided by
drupal_get_module_schema()
is vital for my use case, which is NOT test code. Removing the functionality is a regression, and the above assumption that it is only needed in tests, or not needed at all, is faulty.#2820696: Allow schema inspection for all tables created with a valid schema is an old issue that proposes a solution that is relevant here.