Meeting will happen in #d9readiness on drupal.slack.com.
Gabor Hojtsy (he/him) |
@mikelutz suggested this topic |
mikelutz |
I’ve started work on in an attempt to provide a UI for situations where the migration path can’t be determined without some input from the user. |
mikelutz |
I’m hopeful I’ll have an initial patch soon, but I don’t have much time to work on it right now. I would love to hear any comments or thoughts on the idea in the issue though so I can take them into account as I build it. |
mikelutz |
I’m hopeful, if done correctly, that it will help create a way to solve some of the hard problems that come from trying to migrate a D7 site, both in core and contrib. |
Gabor Hojtsy (he/him) |
@mikelutz sounds fantastic cc @webchick :slightly_smiling_face: |
Gabor Hojtsy (he/him) |
also I think its important to agree whether we are looking at informing people about possible problems or resolving the problems (for the extent of the migration run) |
mikelutz |
Well, we have a system for informing people already, but it’s only used for one audit, so it’s not really used much. |
mikelutz |
My goal is to be able to accept some user input to resolve issues, I’m currently trying to define how that can work pretty strictly though, so this doesn’t turn into a huge hack. |
mixologic |
How much difficulty do people encounter with custom modules on d7 sites? |
mixologic |
If we lived in a perfect world and had perfect migrations for all of core and contrib, how much of an effort is it for folks to upgrade whatever they've custom built? |
mikelutz |
There are issues just doing a pure core migration that need some level of user input to resolve. |
Gabor Hojtsy (he/him) |
@mixologic custom modules can keep their custom data storage mostly (db), their config, etc. would need to move |
Gabor Hojtsy (he/him) |
@mixologic custom modules integrating with contrib/core stuff that changed storage need their own migrations |
mixologic |
Do we have an idea as to how big of an impact that is? |
Gabor Hojtsy (he/him) |
@mixologic I don’t think that will be the deciding factor on top of the core/contrib migrations sites already need to do… in your ideal world where those are solved, there are some nice tutorials for writing migrations :slightly_smiling_face: |
mikelutz |
I’m not sure how to quantify it. Custom code for D7 needs to be rewritten for D8, and as with contrib, if you want to move custom data from d7 to d8 you need to write a migration path for it. |
webchick |
Yeah, it’s hard to make a blanket statement across all of custom modules because custom modules can be so varied. It could be a simple “glue” module between two other contribs to e.g. simplify forms, force options… in which case there’s basically no data. Or it could be a completely custom integration with some external storage system, in which case also no data migration because you are leaving that storage intact. |
webchick |
But huge +1 to the general idea of “if we prompt users for some basic info their migrations are 30,000x more successful” movements. :smile: |
mikelutz |
I think even keeping it to a boolean, ‘do this or that’ per audit plugin might be enough. |
mixologic |
This is probably offtopic then, but the updates data we get from sites *does* include everybodies custom modules. It'd take some datamining, but we could potentially determine just how much custom code d7 sites rely on. (well, at least just a 'quantity of modules') no telling what those modules are or how big they are. |
mikelutz |
replace php_code filter with filter_null, or just skip it? Set all conflicting text areas the same, or split them? |
mikelutz |
@mixologic That might be a good thing to know, but I’m not sure how much it will help. |
mixologic |
really just an overall picture of understanding the d7->d8 upgrade/migration blockers. |
Gabor Hojtsy (he/him) |
@mixologic that sounds like a very great data to look at for the migration landscape @webchick is all around on :slightly_smiling_face: |
mixologic |
yeah, i.e. offtopic but I figured it'd be interesting data to know. |
webchick |
Yeah, would be great. Even not necessarily mining custom modules, but determining # of D7 sites even _eligible_ to move off of D8 (all their _contrib_ modules are ready, with migration paths) would be huge. |
webchick |
Because I’m guessing that number hovers around 20% at best. Everyone else needs to hire an expensive dev |
webchick |
Or 6 :smile: |
mixologic |
or 100 inexpensive devs. |
mixologic |
and then 10 expensive ones later |
Gabor Hojtsy (he/him) |
:facepalm: |
mikelutz |
I hate to say it, but some of those 80% just need to be connected with a qualified dev to help their migration. |
chx (he/him) |
@mikelutz I had some discussions around this audit with @webchick before |
chx (he/him) |
My opinion was -- still is -- this is best done in D7 and not in D8/D9 |
chx (he/him) |
`import __future__` from python, sort of :slightly_smiling_face: |
chx (he/him) |
We might be able to do something in D8 but isn't this much easier in D7? |
Gabor Hojtsy (he/him) |
@chx (he/him) it could still be though that hte D8/9 site has issues, eg. permissions to some files are different or the input filter module was not added to the D8/9 site taht was totally fine on D7 |
mikelutz |
I wouldn’t think so. You might be thinking about trying to fix corrupted data in d7 prior to a migration, and that might not be a bad idea. I’m more concerned with the known issues where there are storage or configuration incompatibilities between a correctly implemented D7 site and D8. Those need to be managed with a user choice, and the correct time is during the migration. |
chx (he/him) |
Ah |
Gabor Hojtsy (he/him) |
so we need to do the auditing either way on the D8/9 side |
chx (he/him) |
that is very interesting problem -- it might have been solved since , I am not sure , everything I remember of core work is years old -- we discussed passing configuration into migration and it was never properly done IMO |
chx (he/him) |
is there a good mechanism for that? |
mikelutz |
What do you mean exactly? to what configuration are you referring? |
Gabor Hojtsy (he/him) |
Topic proposed by @chx (he/him) :slightly_smiling_face: |
xjm |
I mentioned this above, but these are unlikely to be un-deprecated |
Gabor Hojtsy (he/him) |
I did not say the topic is undeprecating them :wink: |
xjm |
:slightly_smiling_face: |
valthebald |
there could be (contrib) module literally called `deprecated`, that adds those functions back |
Gabor Hojtsy (he/him) |
that’s such a slippery slope :smile: |
valthebald |
it is :slightly_smiling_face: |
valthebald |
but it has 2 advantages over "un-deprecating":
- this is out of core, so noone really cares
- those who _do_ care can "standardize" in using one module over reinventing the wheel in every script |
Gabor Hojtsy (he/him) |
well, I think simple DX could be part of if not in core |
Gabor Hojtsy (he/him) |
rather than a blanket “Deprecated module” |
valthebald |
sounds good |
chx (he/him) |
that's extremely heavy :confused: |
chx (he/him) |
the entire entity module just to have a few wrapper functions? |
Gabor Hojtsy (he/him) |
project/entity_wrapper_functions? :slightly_smiling_face: |
Gabor Hojtsy (he/him) |
the entirety of all the deprecated things would also be heavy :smile: |
chx (he/him) |
mmmm |
chx (he/him) |
deprecated wrappers wouldn't be that heavy |
chx (he/him) |
I might be willing to co-maintain something like that |
webchick |
To play devil’s advocate… since those are “primary” actions one wants to do with entities, and therefore presumably lots of core + contrib modules want to call them… why would we not have easy wrappers (fine if they’re OO) in core? |
webchick |
This would also make transition from 7 -> 8/9 easier if these calls did not require a contrib module dependency |
valthebald |
as in "optional core" module? |
webchick |
No, as in a simple OO replacement for entity_load() et al seems like it ought to be part of the primary entity API and not require a contrib module. |
webchick |
Although makes it sound not too terrible |
mikelutz |
There are proper replacements in core for all of those, using injectable services. |
webchick |
Yeah, so I guess more info is needed on the request then… @chx (he/him) what were you hoping for? Literal procedural wrappers? Or an easier include path or? |
chx (he/him) |
literal procedural wrappers |
chx (he/him) |
look here this is something i cobbled together today quick |
chx (he/him) |
in production, no IDE, nothing |
chx (he/him) |
this is very typical |
webchick |
Hm. Well if it’s literal procedural wrappers then yeah, probably contrib for that. :slightly_smiling_face: If it were “replace entity_load with Entity::load()” (or whatever) I could get behind that |
xjm |
We did that already, and the discovered it was a bad idea :slightly_smiling_face: |
mikelutz |
Entity::load() already exists. entity_load was just a wrapper around it. |
chx (he/him) |
yeah but then you need to find the namespace for it and whatnot |
webchick |
Oh? What was the bad idea about it? |
mikelutz |
That script might have taken a few more characters to use the correct services, but it would have used the same number of lines to use the correct storage object.s |
xjm |
It's not yet deprecated or anything |
xjm |
But that's been there since before 8.0.0 |
webchick |
Yeah, that seems sensible to me. :slightly_smiling_face: |
mikelutz |
`$file = entity_load('file', reset($fids));` literally becomes `$file = \Drupal::entityTypeManager()->getStorage('file')->load(reset($fids));` |
webchick |
I’m +1 to the general idea of a “deprecated procedural wrappers” contrib module FWIW. Anything that makes D7 => D8 easier. :slightly_smiling_face: |
xjm |
This is all pre-8.0.0 stuff |
chx (he/him) |
@mikelutz then -- I am not insane or sarcastic, I am serious -- could we deprecate `\Drupal::entityQuery()` ? |
chx (he/him) |
@mikelutz that way everything channels into the storage handler. |
chx (he/him) |
we don't have Drupal::entityLoad so why Drupal::entityQuery, then? |
mikelutz |
@chx (he/him) Possibly? The storage handler does have query access. |
chx (he/him) |
yes, entityQuery is `return static::entityTypeManager()->getStorage($entity_type)->getQuery($conjunction);` |
chx (he/him) |
because then already for querying one would grab the storage handler once and call query - load - save |
chx (he/him) |
that is smooth and easy |
chx (he/him) |
i can see this work, pattern and habit and all that |
mikelutz |
Yes, that should probably be deprecated, but double check with @berdir to see if there is any reason that function is still there that I don’t know about. |
mikelutz |
there used to be an entity.query service that function used to return, but the service was deprecated in favor of getting the query from the storage. I expect that method should have been deprecated at the same time. |
mikelutz |
131 usages in core of that method. not bad considering.. |
berdir |
We kept that ob purpose, not what we'd get from removing it, it goes through storage and you get a type hint. Also, we have Entity;;load() |
mikelutz |
Gotcha. I don’t think I’ve ever used it, it just seemed like an unnecessary wrapper, but I suppose it doesn’t hurt anything. |
mikelutz |
Still, @chx (he/him) I would say best practice is to send everything through the storage object. |
chx (he/him) |
that's what i suggest too |
chx (he/him) |
if we are nuking entity_load then nuke entityQuery too for the sake of consistency |
berdir |
If you can inject or already have the storage, sure, use that. But imho useful for tests and hooks and so on. Also, if we remove that, should we remove remove config()? Where's the difference? |
catch |
Entity::load() doesn't actually work to load arbitrary entities, it just makes Node::load() and friends work. |
chx (he/him) |
@berdir the question above. Why don't we have Drupal::entityLoad but we have Drupal:::entityQuery? They are the exact same. |
berdir |
because we have Entity::load() which covers that, with the exception of a dynamic entity type, which I think is an exception |
berdir |
I mean, I don't feel that strongly about it, I just don't see the point of doing the work of deprecating that, imho the benefit of deprecating functions in include files so we don't have to load them anymore is bigger |
chx (he/him) |
I certainly have no strong opinion. Maintaining a deprecated wrapper module is little work because it would do .... nothing. |
chx (he/him) |
it's just functional wrappers. |
chx (he/him) |
I was just seeking consistency and easy scripting but ... not feeling that strongly. |
webchick |
We could even give it a fun name like “wrappymcwrapperson” :wink: |
chx (he/him) |
WrappyMcWrapperFace :stuck_out_tongue: |
Gabor Hojtsy (he/him) |
suggested by @Mile23 |
xjm |
So I mentioned this last meeting, but #20 is misleading. It says: |
xjm |
> Committers discussed our priorities for D9 and it was unanimous that we don't want to support WebTestBase in D9... |
xjm |
But (a) I'm a committer and was not present (b) even though no one _wants_ to support it I think it's too disruptive/too late now to deprecate it for removal in 9.0.0 |
xjm |
We need people's tests to not break when they're testing their upgrades to D9 |
Mile23 |
specifically WTB? |
xjm |
Well WTB is most important probably, yeah |
Mile23 |
because 'simpletest' means a bunch of things. |
xjm |
But the UI, the whole shebang |
xjm |
I think we don't want to support it but rewriting tests from WTB to BTB isn't trivial work and so I'm reluctant to make that deprecation so late |
xjm |
It absolutely, definitely should not exist in 10.0.0 |
Mile23 |
ok, so until drupal 10 the quality chain for Drupal will remain unmaintainable? |
xjm |
That's not at all loaded :stuck_out_tongue: |
Mile23 |
:slightly_smiling_face: |
xjm |
I think @catch might have some thoughts or info here too |
xjm |
Or @mixologic e.g. at a minimum we would need CI to automatically install simpletest from contrib if the test suite is simpletest, in which case there's actually even more risk of unmaintainability |
Mile23 |
That's why we should deprecate it formally. |
Mile23 |
like, today. :slightly_smiling_face: |
xjm |
I think it's too late |
xjm |
I think it's been too late for all of 2019 |
mixologic |
CI installing simpletest is no big deal, but yeah, tricky. |
Mile23 |
ok, well it would have been nice to know that in january. |
xjm |
Well, my position on it has not changed :slightly_smiling_face: I've been saying since last fall I thought this would be the one thing we should formally deprecate for D10 instead of D9 |
xjm |
But I was not consulted for the comment @larowlan posted |
Mile23 |
Not on issues, though. |
xjm |
I wasn't aware of larowlan's comment at all until two weeks ago |
mixologic |
Other data: the phpstan results show that there are 591 tests in d8 contrib that are extending WebTestBase. |
xjm |
Because I was surprised that someone would post that decision as "unilateral" |
Mile23 |
can we open a d10 branch so that I can take all the work I've done and postpone it for that? |
xjm |
@mixologic Is that direct children or all descendants? |
xjm |
If it's direct children that's great news, but very surprising |
catch |
I still think we should deprecate it and move it to contrib, especially if we can bring it back in for contrib testing as a transition thing. |
mixologic |
@xjm Im unsure if phpstan figures that out. |
Mile23 |
If you want simpletest module as contrib then someone hire me and I'll do it before the end of next week. |
mixologic |
@xjm also showing those 591 tests are spread across 335 modules. |
Gabor Hojtsy (he/him) |
(sidenote: yay for having this contrib data to mine for these discussions :) |
catch |
That's a lot more than zero, but considering there's a year to go. That's 335 modules with on average 1.5 tests each to convert. |
xjm |
It would be good to check an individual module that we know extends a child of WTB in the results so we know whether those are complete |
Mile23 |
So do we want to update the roadmap with the idea of deprecating simpletest in favor of simpletest_legacy contrib? |
mixologic |
are there children of WTB in core, or do you mean, extends WTB in the module, and then extends that in the module? |
mixologic |
Also, those numbers might even be high. |
catch |
theoretically any contrib module could extend any core test. |
catch |
In fact entity_cache does this for several core tests iirc but that's never going to get ported to Drupal 8. |
catch |
Let alone 9. |
mixologic |
right, but the only core tests extending webtestbase are the tests of webtestbase themselves right? |
Mile23 |
right. |
Mile23 |
so if we did simpletest_contrib those tests would move. |
Mile23 |
So shall I start work on simpletest_contrib? or what? Can maintainers please offer non-contradictory direction on this? |
xjm |
I think it depends on whether and how much infra work it would take to make CI understand the whole inheritance chain and install simpletest when it's needed |
mixologic |
Depends on how we do it. |
xjm |
And even then I'm worried about the chicken-and-egg problem of tests failing because you're using removed code and tests failing because the _test API itself_ is removed code |
mixologic |
If we make simpletest something installable with composer, then the only thing you need to do to the CI chain is nothing. |
Mile23 |
there are a few issues here.... 1) there's a module called simpletest in core and it should be deprecated. 2) there's a class called TestBase and it should be deprecated. 3) There's a simpletest UI that might end up living in D9 as testing_ui. 4) There's a test runner called run_tests.sh that needs to be refactored for all of the above. |
mixologic |
And basically communicate to maintainers "hey, do you still have simpletest tests? those are deprecated. you can keep using them in your module by doing a `composer require drupal/legacy-old-you-should-convert-simpletest` |
Mile23 |
word from @alexpott is that run-tests.sh will live on, so if we're to do the other things, we need to refactor it. |
Mile23 |
The roadmap I wrote does a lot of this refactoring. |
Mile23 |
if simpletest were to become contrib, we'd need to do that refactoring. |
alexpott |
Well my word is based on the fact that we’ve not got something that does what we need written another way. |
Mile23 |
in order to just run tests. |
Mile23 |
ok, so yah, then we still don't know what the testing system for D9 is supposed to look like. |
catch |
@mixologic is the contrib data just from the latest branch of contrib projects or all branches? |
mixologic |
as far as ci/infra is concerned, the only thing we need is a small wrapper around phpunit that can run them in parallel, and figure out if a test just flat out crashed, and report accordingly. |
mixologic |
@catch that is from the latest branch of contrib projects that have a 'supported release' |
catch |
@Mile23 I would think we could continue with the refactoring regardless of what exactly happens with deprecation but is that a wrong assumption? |
Mile23 |
@alexpott so are we doing something other than run-tests.sh for D9? |
Mile23 |
@catch well I have issues that I wrote in 2017 you can review. :slightly_smiling_face: |
xjm |
Right, I agree with @catch; whether the deprecation is for 9.0.0 or 10.0.0 doesn't need to block low-level rearchitecting work |
Mile23 |
2015 I mean. |
Mile23 |
dutifully rerolled up through 2018. |
alexpott |
I don’t think we’ve got anything on the table yet to replace run-tests.sh - if we do great but I worry about how long it takes to get stuff like that right. |
Mile23 |
there was that issue about simple-phpunit or whatever i's called. |
mixologic |
Honestly I think we could take the existing run-tests.sh, strip out most of it, but keep the essential parts, and 'maintain' a second testing infrastructure. |
catch |
@Mile23 right so not actually any need to postpone to Drupal 10, we need to do that stuff in order to be able to deprecate regardless of version. |
alexpott |
Well simple-phpunit was about trying to make our tail not be wagged by PHPUnit deprecations - but I hit a really hard problem with listeners and error handlers. |
Mile23 |
in order to keep run-tests.sh running, we need some of the functions from simpletest module. |
mixologic |
copy them |
Mile23 |
yah, 'refactor.' :slightly_smiling_face: |
xjm |
@Mile23 So you're more concerned about `run-tests.sh` itself and the architecture at that level, rather than `simpletest\TestBase` children? |
Mile23 |
yes. |
Mile23 |
because it's a mess, and we could make it maintainable. |
catch |
@Mile23 because it blocks removing simpletest or just because it could be better? |
Mile23 |
why or? :slightly_smiling_face: |
xjm |
heh, well put |
Mile23 |
so is there some place to plan this that's better than that roadmap issue? |
xjm |
I think the roadmap issue is a good place -- thanks for your work already on it |
Mile23 |
is there some place this conversation can continue? |
mixologic |
yeah, I believe the key thing here is whether or not its deprecated is decoupled from whether or not it can move forward. |
Mile23 |
maybe please put it on a maintainer meeting agenda or something? |
mixologic |
Though I can see a concern being that if its *not* deprecated, then its *not* going to move forward. |
Mile23 |
@mixologic I think that's not correct, because decoupling has been in the issue queue since 2015. |
Mile23 |
anyway, thanks. I'll shut up now. |
Mile23 |
:slightly_smiling_face: |
larowlan |
in my defence, ‘larowlan’s comment’ was not a personal opinion but rather a consensus decision from a committers meeting |
larowlan |
My personal opinion is its not too late to deprecate WTB |
Mile23 |
Updated the roadmap issue with some takeaways from this conversation: |
Comments
Comment #2
Gábor HojtsyComment #3
ndobromirov CreditAttribution: ndobromirov at FFW commentedComment #17
Gábor HojtsyPosting meeting log and first set of participant credits.
Comment #21
Gábor HojtsyCredited the remaining folks. Thanks all! And special thanks to @mlhess for the logs.