Meeting will happen in #d9readiness on drupal.slack.com.
| Gábor Hojtsy (he/him) |
@ccjjmartin sort of raised this topic specifically about rector generated patches but I think it applies regardless of how you generate the patch |
| Gábor Hojtsy (he/him) |
@berdir wrote a whole docs page about this :slightly_smiling_face: |
| Gábor Hojtsy (he/him) |
in short: ideally the project has a D9 info specified on their project page |
| Gábor Hojtsy (he/him) |
eg. go to https://www.drupal.org/project/upgrade_status |
| Gábor Hojtsy (he/him) |
where it saysDrupal 9 is planned to be released June 3, 2020.Deprecated APIs are removed up to 8.7.x. #3100361: Remove usage of file_directory_temp() once Drupal 8.8.x can be required remains. Tests need to be updated as well: #3100308: Drupal 9 compatibility of Upgrade Status (tests)(edited) |
| Gábor Hojtsy (he/him) |
the second part is what project maintainers specify |
| Gábor Hojtsy (he/him) |
this has been used with several hundred of projects |
| ccjjmartin |
Rector is a moving target so it isn't to say that if I ran it today it would give the same results tomorrow (more deprecation's covered). Also, it produces patches that don't make a module fully D9 ready. So it seems like a discussion needs to be had here as patches will become both outdated and appear uncomplete. |
| Gábor Hojtsy (he/him) |
@ccjjmartin I think that is true regardless of how the patch was created |
| Gábor Hojtsy (he/him) |
a human will not make patches fully complete on their first attempt either :smile: (edited) |
| ccjjmartin |
We could generate separate patches for each deprecation as a way to get the patches into a mergeable complete state (set scope). |
| Gábor Hojtsy (he/him) |
the key thing is the project maintainer may have different preferences |
| Gábor Hojtsy (he/him) |
@ccjjmartin this TOTALLY depends on the maintainer, how they want to deal with stuff |
| Gábor Hojtsy (he/him) |
some people will want to deal with deprecated API use up to 8.7 for now, regardless of which deprecation it is |
| Gábor Hojtsy (he/him) |
some people will want to deal per deprecated API |
| Gábor Hojtsy (he/him) |
some people will tell you to go away and not bother because they don’t have time for this yet |
| shaal |
@Gábor Hojtsy (he/him) maybe we should publish somewhere the suggested way of working?or maybe list all the different ways one can get their module/theme ready for D9?(or is that what's already in the doc you made? do you have a link for that doc?) |
| Gábor Hojtsy (he/him) |
@shaal for project maintainers there is https://www.drupal.org/docs/9/how-to-prepare-your-drupal-7-or-8-site-for... |
| Gábor Hojtsy (he/him) |
that links to https://www.drupal.org/node/3032484 which Berdir created |
| Gábor Hojtsy (he/him) |
I think that could be updated with scenarios |
| Gábor Hojtsy (he/him) |
But it comes down to this from there which is the same as I said above:Make a suggestion on how to split up the changes. […] Give the maintainer some time to reply to that, if there is no response after a while you can still provide patches for your suggestion, but it might take a long time for them to get committed. |
| Gábor Hojtsy (he/him) |
I don’t think this is gonna be one good way[TM] (edited) |
| Gábor Hojtsy (he/him) |
@tr raised this topic |
| Gábor Hojtsy (he/him) |
Specifically about #2932462: Add EntityContextDefinition for the 80% use case |
| timplunkett (he/him) |
the new API is only for cases where the data type started with entity: |
| timplunkett (he/him) |
a generic entity condition should be unaffected |
| tr |
Specifically, this change made in D9 will completely prevent Rules from working in D9 with no known workaround. I want to discuss how to get this fixed before D9 is released. |
| tr |
@timplunkett (he/him) The problem is there is no longer a factory method for creating ContextDefinitions. Which means programmatic manipulation of context is impossible. And that's fundamental to Rules. |
| timplunkett (he/him) |
ContextDefinition::create('entity') |
| timplunkett (he/him) |
as I posted on the issue |
| tr |
It's a bigger problem than that ... I have worked on it quite a bit, and I don't see a way forward |
| timplunkett (he/him) |
Please explain the problems on the issue. You don't have any steps to reproduce or descriptions of what is affecting you |
| timplunkett (he/him) |
but generic conditions and generic entity conditions absolutely still work |
| timplunkett (he/him) |
and there is test coverage for them |
| tr |
I'm trying to focus on the bigger picture. If you put functionality into entities rather typed data, then this breaks manipulation of typed data. I should be able to say ContextDefinition::create('entity:node') for example. or ContextDefintion::create($datatype) for any typed data datatype. And that's what I used to be able to do before this change. Now, I have to say if $datatype then do something different, but how do I programmatically know that a specific datatype needs to do something different? The entity system is now breaking that contract. |
| tr |
The issue was supposed to be just about convenience functions to save keystrokes, but by deprecating the constructor for entities and only entities, it has removed the ability to use datatypes in a type-agnostic way |
| timplunkett (he/him) |
That's not true |
| timplunkett (he/him) |
It was never about convenience |
| timplunkett (he/him) |
It was a follow-up to a crucial bug fix |
| timplunkett (he/him) |
If you want to propose a change to how this system works in D9, that's fine. If you want it reverted, it needs to be deprecated now and removed in D10. But this exists in 8.6 and onward already |
| tr |
"It has long been possible to create a context object wrapping around an entity. However, the syntax was rather cumbersome, and is now deprecated" |
| timplunkett (he/him) |
Read #2671964: ContextHandler cannot validate constraints for the original bug fix |
| timplunkett (he/him) |
this was a followup issue |
| timplunkett (he/him) |
but considering that this logic exists in all supported versions of D8, how is it a new problem now? |
| tr |
It isn't new per se, but with 25000 deprecation notices generated by changes in D8.6, 8.7, and D8.8, and with a BC layer in place for D8, the impact of this on Rules was obscured until I could get around to fixing thousands of lines of Rules code to accommodate these changes in core. |
| tr |
If you want to talk about the constraint issue, that's fine too - why is that in Entity? Constraints as functionality should be available to all typed data, not just Entity. |
| timplunkett (he/him) |
it is |
| timplunkett (he/him) |
Have you read the code? It's not in Entity |
| tr |
Regardless, Rules will not work in D9 right now. The above is the only blocker, and I don't know how to fix it and I don't know how to go forward with getting accommodations for this in D9. |
| timplunkett (he/him) |
See also #3008431: A context object for a specific entity type will not match a generic requirement for any entity which was fixed |
| timplunkett (he/him) |
Well that's fine, can you either expand your bug report to explain the actual problem? Or link to a Rules issue explaining it |
| tr |
@timplunkett (he/him) Getting away from the blocker ... but your argument was that the critical issue was putting constraints into entity which is why EntityContextDefinition was needed. But if it's not an entity-specific functionality then perhaps that could be put into ContextDefinition ... |
| timplunkett (he/him) |
\Drupal\Core\Plugin\Context\EntityContextDefinition::getConstraintObjects() |
| timplunkett (he/him) |
\Drupal\Core\Plugin\Context\EntityContextDefinition::getSampleValues() |
| timplunkett (he/him) |
those two functions are why we need the subclass |
| timplunkett (he/him) |
to encapsulate the entity-specific logic |
| timplunkett (he/him) |
while allowing ContextDefinition (or any other subclass) to have its own logic |
| timplunkett (he/him) |
all of this being called from \Drupal\Core\Plugin\Context\ContextDefinition::isSatisfiedBy() |
| tr |
But why remove ContextDefinition::create('entity:' . $datatype) ? That create() acted as a factory which works with all datatypes. |
| tr |
And that deprecation/removal wasn't really discussed in the issue |
| timplunkett (he/him) |
if your proposal is "bring back the BC layer, and stop calling it a BC layer", that's something to be discussed |
| timplunkett (he/him) |
because then it will match \Drupal\Core\Annotation\ContextDefinition::getDefinitionClass() which was not removed |
| tr |
@timplunkett (he/him) That would be one easy solution - just don't remove the create(). |
| timplunkett (he/him) |
create wasn't removed |
| tr |
ContextDefinition::create('entity:' . $datatype) was removed. |
| timplunkett (he/him) |
"removed" doesn't mean anything |
| tr |
ContextDefinition::create($datatype) still exists and works for all datatypes EXCEPT entities. |
| timplunkett (he/him) |
you mean it fails an assert() call, yes |
| tr |
OK, the ability to use create() was removed if you're trying to create a ContextDefintion for an entity datatype, is what I'm using "removed" for as a shorthand, because I thought that was understood - that's where the trigger_error comes in |
| tr |
Regardless, given a datatype, there is no way to programmatically know how to create a ContextDefinition for that datatype any longer. |
| timplunkett (he/him) |
well there is, you just don't want to special-case entities |
| timplunkett (he/him) |
which I respect |
| timplunkett (he/him) |
but please don't say "there is no way" when there is a way you just don't agree with |
| timplunkett (he/him) |
I am posting a proposal to your issue |
| tr |
Not just that, but what if someone else declares a custom datatype - is ContextDefinition guaranteed to work for that? |
| timplunkett (he/him) |
aha |
| timplunkett (he/him) |
so we're missing a way to map from data type to the appropriate ContextDefinition class |
| tr |
And no, it's not just special casing. That's obviously the first thing to try, and I don't see a way to make that work in Rules. Part of the problem is because Rules needs to extend Context, and creating a special subclass of EntityContextDefinition breaks the ability to easily extend Context. What it's coming down to is an almost complete duplication of the Context code put into Rules just to get around this one little change. |
| timplunkett (he/him) |
that would have been helpful information to put in the issue |
| timplunkett (he/him) |
#3126747: ContextDefinition::create() can no longer be used with an entitytype-specific datatype (like entity:user)#comment-13551945 |
| tr |
Yes. "missing a way to map from data type to appropriate ContextDefintion" |
| timplunkett (he/him) |
okay so let's work on that. because even if what i just posted helped, that's now TWO places we have to hardcode that logic, and in a way that contrib cannot extend |
| tr |
And I can see several ways to deal with that, and to allow extensions to the Context system, but they are all much bigger changes to core, and the sort of API changes that I don't think are possible at this point for D9.0. |
| timplunkett (he/him) |
even still, open the issue for 9.1 |
| timplunkett (he/him) |
just because we can't do it now doesn't mean we shouldn't do it |
| tr |
That's why my focus is on finding out how to partially revert the change for D9.0 so Rules can continue to exist. Either that or I just throw in the towel entirely and give up on Rules. |
| timplunkett (he/him) |
but you are describing two separate things |
| timplunkett (he/him) |
one is the pain of the removal of the BC layer |
| timplunkett (he/him) |
but the other is about the existence of ECD completely |
| timplunkett (he/him) |
those are very different problems that require very different approaches |
| tr |
I have no problem with ECD existing - that's just basic OO, a subclass to provide more specific functionality. And in principle any type could have its own context definition subclass. Similar to DataDefintion. But with @DataType we have a way in the annotation to specify the definition_class while we don't have that with context. And with DataDefintion we can still create($datatype) for ANY datatype, even if that datatype has its own DD subclass. (edited) |
| timplunkett (he/him) |
did you see the patch I posted? |
| tr |
Yes, but it will take me some time to see if that's sufficient. |
| andypost |
I could be wrong but deja vu reminds me conditions on rules and #2011038: Use Context system for actions |
| tr |
@andypost: Wow, that's an old one. Rules actually gave up on core Actions more than 5 years ago and forked its own copy (RulesActions). I don't see much in common with my current issue other than Rules is a major consumer of Context so is easily affected by any changes in core that only consider core needs when making/changing the API. Although Rules has extended the core Context class, it is mostly for minor things that have an open core issue but have never progressed very far. Of these things, the only one I think is essential to get into core is #2329937: Allow definition objects to provide options I plan to try to push that forward in D9.1 as we currently have a work-around in D8. All of these issues fell off the radar years ago when @fago dropped out of Rules development. |
| andypost |
@tr thanks for pointer, I recall contexts matured along in d8 and maybe for 9 it could be "unified". FYI page manager and ctools are still not using core contexts too |
| Gábor Hojtsy (he/him) |
@mixologic raised this |
| mixologic |
Hi, hoping to understand how critical this is, what it blocks, etc |
| Gábor Hojtsy (he/him) |
#3107927: Update Node.js to a more recent version on DrupalCI is the issue |
| mixologic |
So, what does “drop support” mean in core? |
| mixologic |
I guess Im trying to figure out how to make this happen before I leave, and Im wondering what the consequences are if I do not. |
| mixologic |
And I think the conundrum is the attempt to make it use more than one version of nodejs. |
| dan2k3k4 |
ah specifically nvm install 8 && nvm install 10 && nvm install 12 :thinking_face: |
| dan2k3k4 |
Are 8 and 10 really needed? |
| mixologic |
I can make it work if we simply upgrade nodejs to be a newer version and not worry about trying to keep node 8 |
| mixologic |
Will node 12 work with our existing builds for 8.7.x etc. |
| mixologic |
or is there something inherently different that requires different versions on different platforms? |
| dan2k3k4 |
Unless it's for previous Drupal versions? 7 etc.? |
| Gábor Hojtsy (he/him) |
I think @lauriii would now better, and he is not around today as far as I’ve seen. But the real question is whether 10 is compatible with the stuff we do in drupal 8 too yeah |
| mixologic |
or 12 |
| mixologic |
I think the key here is that nodejs is not a core requirement |
| mixologic |
Its a core dev tool, and as such, there should not be any sort of BC concerns. |
| mixologic |
Unless, upgrading to node 12 means that the whole nightwatch framework changes or something. |
| Gábor Hojtsy (he/him) |
Yeah if tests start to badly fail, etc. |
| Gábor Hojtsy (he/him) |
I don’t know the answer, I tagged Lauri above, so he may be able to async answer later tomorrow :slightly_smiling_face: |
| mixologic |
That would be helpful. Because the direction that the patch is going isnt going to work, because we dont have a good way to make drupalci change the environments in drupalci.yml |
| dan2k3k4 |
FWIW: older Gulp or Grunt configs which use node-sass or core-js have caused issues for us when we used Node 6 or 8 and tried to upgrade to 10 or 12. We had to fix the older dependencies and then things would work again.So my assumption is that if we upgrade everything to Node 12, some things might break somewhere down the line, and we'd have to upgrade the places which use those older dependencies.I'm not sure how DrupalCI uses Node, whether it's required for saying building a theme when it's testing a theme... but if it is, then it could cause issues with the theme not building at all or causing the theme to build without certain things working (e.g. some @keyword doesn't get compiled so the built CSS becomes invalid with random @keyword instead of the correct compiled output) |
| mixologic |
DrupalCI uses it to run yarn install, which puts all the nightwatch testing infra into place. |
| mixologic |
And then runs nightwatch js tests. |
| mixologic |
But otherwise, it does not use it to compile sass, or do any other stuff. |
| dan2k3k4 |
Mmm then I think it should be fine to run on one node version (ideally the latest LTS) |
| dan2k3k4 |
It's not like PHP where we test 5.6, 7.0 etc. |
| mixologic |
I suppose theres a chance that it could break nightwatch tests somehow |
| dan2k3k4 |
What does it currently run? node.js v6? or v8? (edited) |
| dan2k3k4 |
https://blog.risingstack.com/update-nodejs-8-end-of-life-no-support/The problem is that those versions of your dependencies that worked fine with Node 8 might not be compatible with the V8 / NAN version in later @nodejs versions.https://github.com/nodejs/nan/issues/504Does Nightwatch use NAN anywhere?When I search .I only get \nAn for new line inside a text string...https://github.com/nightwatchjs/nightwatch/search?q=NAN&unscoped_q=NAN (edited) |
| mixologic |
its currently using node 8 |
| dan2k3k4 |
mmm Lauri's last comment was to this issue: [#3124107]Is that going to possible? I don't think it's necessary to select Node version if we're running just Nightwatch and not building CSS or anything (edited) |
| lauriii |
IIRC we had some concerns on upgrading to Node.js 12 after I posted patch for that #3107927: Update Node.js to a more recent version on DrupalCI#comment-13434813 |
| lauriii |
the concern was that it would be possible that contrib projects using DrupalCI for doing JavaScript testing would break if we upgraded to Node.js 12 and we would have to do research how bad the break would be |
| lauriii |
at the time it seemed like it would be hard to find resource for doing that research since it wasn’t something I would be capable of doing |
| lauriii |
so I proposed a patch which would allow us to run multiple versions of Node.js |
| lauriii |
which would allow core to upgrade to Node.js 12 before we move rest of the projects to that version |
| lauriii |
but given #3124107: Allow selecting Node.js version with nvm#comment-13547182 it seems like that would be more difficult than I initially thought |
| dan2k3k4 |
I'm still unsure how Nightwatch tests would fail going from Node v8 to v12 :thinking_face:Do we have an overview of some of the more complex tests being run somewhere?Maybe in the worse-case outcome, if tests fail, they should be upgraded to work for v12?I don't see anyone else in the Nightwatch issue queue complaining that they've had to upgrade tests when they've upgraded Node version but it's likely that those people kept the Node version up to date, and/or did not have any "complex" tests? |
| Gábor Hojtsy (he/him) |
@lauriii summary of that history would be key to post on the issue, so we can refer to it later |
| lauriii |
@dan2k3k4 Nightwatch tests contain JavaScript code which could depend on features that are specific to the Node.js version. This extends to all yarn dependencies and there are packages that doesn’t work with Node.js 12 without updating. Some of these packages cannot be updated now because those releases might not support Node.js 8. This is the case for core at the moment for example; there are package versions that are not compatible with Node.js 12 but we cannot update to them unless we drop support to Node.js 8 |
| lauriii |
at the moment, Babel might be the only one facing this problem |
| lauriii |
luckily Babel is not run on DrupalCI by Drupal Core so it shouldn’t lead into problems even if we upgraded to Node.js 12 |
| lauriii |
DrupalCI also allows running custom commands which could allow more advanced setups |
| dan2k3k4 |
So the custom commands, would this mean someone can setup a module/theme that requires say a build step using for example an older version of Grunt... and if DrupalCI only installs Nightwatch, then it would need to also install Grunt by running npm install for the modules package.json file? |
| dan2k3k4 |
Or are the custom commands more related to running something like a macro for Nightwatch? |
| lauriii |
We install yarn dependencies part of the build process. You can search for yarn install on https://dispatcher.drupalci.org/job/drupal_patches/28066/consoleFull for example |
| lauriii |
We have used custom commands for running different linters etc for example in the Claro contrib project |
| lauriii |
https://git.drupalcode.org/project/claro/-/blob/8.x-2.x/drupalci.yml#L9 |
| lauriii |
Hmm maybe the yarn install is only run for core |
| lauriii |
since we’ve manually installed dependencies for the contrib project |
| dan2k3k4 |
Browsing through the dispatcher log, I noticed:fsevents@1.2.9 -> fs / fseventsThat can also cause some issues between Node versions. Although perhaps we should be removing that from yarn.lockhttps://stackoverflow.com/questions/57082249/how-to-fix-error-fsevents2-... adding a resolutions key to the package.json file :thinking_face: (edited) |
| lauriii |
@mixologic let me know if you want to chat or call about this at some point ^ |
| lauriii |
would be awesome to fix this :smile: |
| mixologic |
@lauriii Im around now |
| mixologic |
if you are. Im hoping to figure out what might be the best path forward here. I guess I dont really understand what it means to “drop” support for node 8 |
| lauriii |
I’m available now |
| lauriii |
@mixologic do you want to Zoom or chat here? |
| mixologic |
zoom could work. |
| mixologic |
@lauriii do you have a mtg link? (edited) |
| lauriii |
yup, just a sec |
| lauriii |
https://acquia.zoom.us/j/99669577659 |
| mixologic |
Maybe this will work |
| lauriii |
#3107927: Update Node.js to a more recent version on DrupalCI#comment-13434813 |
| lauriii |
@mixologic did the development build pass yesterday? |
| mixologic |
@lauriii they timed out because our test builds have a really old version of drupalci.yml that doesnt expect the javascript tests to be able to run in parallel |
| mixologic |
So it never got as far as nightwatch. Oh.. but it did get as far as yarn and i didnt look at those. |
| lauriii |
@mixologic anything I can do to help on that? |
| mixologic |
@lauriii Im looking at the console output of https://dispatcher.drupalci.org/job/drupalci_test_containers/29/consoleFull right now, at the yarn install step to verify that it ran on node 12, and succeeded. Can you look as well and keep an eye out for things I might not see as a failure? |
| lauriii |
I’ll check |
| mixologic |
https://dispatcher.drupalci.org/job/drupalci_test_containers/29/artifact... |
| lauriii |
the output from the yarn install looks good |
| lauriii |
@mixologic is there a way to pull the dev image locally? |
| mixologic |
yes. |
| lauriii |
I can run drupalci locally with the dev image |
| mixologic |
its just the tag, basically |
| lauriii |
so it’s this php-7.3.x-apache:dev? |
| mixologic |
yep. |
| mixologic |
except I wouldnt use 7.3.x-apache |
| mixologic |
build: environment: startcontainers: runcontainers: phpversion: ‘php-7.3-apache:dev’ |
| mixologic |
would be better |
| mixologic |
php-7.3-apache:dev |
| lauriii |
okay :thumbsup: |
| lauriii |
Plain Text output.txt28 kB Plain Text — Click to download |
| lauriii |
@mixologic here’s output from running drupalci locally with php-7.3-apache:dev |
| mixologic |
@lauriii that looks good to me afaict |
| mixologic |
but I cant see the colors |
| mixologic |
Do you see anything wrong with that? |
| dan2k3k4 |
bash syntax shows colours |
| mixologic |
Right, its more the issue of converting the [0;32m stuff back into its “pass/fail” colors |
| mixologic |
oh, well, actually I can just “cat output.txt” and I can see it fine in my iterm |
| dan2k3k4 |
:+1: |
| mixologic |
Allright, given that, @lauriii, this looks like we can just upgrade to node12 and not have any impact on core that we know of. |
| mixologic |
image.png |
| lauriii |
I didn’t see anything wrong with the output |
| mixologic |
okay, well, lets pull the trigger. |
| mixologic |
Bang. done. Docker should rebuild the containers, but since they were just built on the same server yesterday, it probably will go very quickly and just push up the tags. |
| lauriii |
awesome, thank you! |
| mixologic |
Yep. already done |
| mixologic |
Okay, so, now we watch core tests. |
| lauriii |
:crossed_fingers: |
Comments
Comment #2
gábor hojtsyComment #16
gábor hojtsySaving first set of notes and credits.
Comment #23
gábor hojtsy