I suspect: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/filte... will be on the list for removal.
I'm not sure of the process here, whether to reopen this or a different issue.
But the CRs on this issue I think is missing some detail on some breaking changes for tests for field ui.
I found this issue when looking at https://github.com/Chi-teck/drupal-code-generator/issues/209 with @chx in slack.
It also seems like some routes and wrappers changed, I think it might be helpful to document those on a CR that makes it clear what changed here.
I'm not sure what other review this needs it's essentially a one for one move to the new classes.
Hooks have to be in the hooks namespace for now until https://www.drupal.org/project/drupal/issues/3481903 ✨ Support hooks (Hook attribute) in components Active
But maybe we do that other one first.
I am on drupal 10.4.7
Level 2 has separate fields for each one.
I think for this case it's better to write something custom.
I'll likely use this module for most other moves since they are almost all just a single level transition.
This module is super impressive!
Sure!
Here is the structure:
- Level 1: Three column component
- Level 2: Image Text Paragraph
- Level 3a: Image Paragraph -> Media reference
- Level 3b: Text Paragraph -> Text field (yes I know this is redundant)
So I created two new fields on level 2 one for a media reference, one for a text field. Both translateable.
I wrote migration 1 from level 3a to the media reference on level 2.
I wrote migration 2 from level 3b to the text field on level 2.
I created new custom field with media and text translateable.
I wrote migration 3 from level 2 media field to custom field media and level 2 text field to custom field text.
This migrated only the english.
Thanks! That isn't quite what I meant, that copies all values from another instance, I want to copy just individual subfields.
I can confirm this is fixed in dev.
nicxvan → created an issue.
I haven't reviewed this since my earlier comment.
Hook implementations are not covered strictly under BC. I know core sometimes still provides it.
I'm not sure either how we would do it in this case.
Tagging for myself later.
Ok I've merged in the changes from the other branch exploration.
I've also rebased on 11.x
High level notes.
We replaced ModernWidget with WidgetInterface.
We added a Generic Element.
We added changetype.
nicxvan → changed the visibility of the branch 3525331-simplify-widget to hidden.
Yep generally the older one wins, but the one worth work should move forward.
I closed the other.
While this one is older the other has an MR already.
There is a related issue I can't find at the moment. I think this needs to be solved with a condition, I'm wondering if it should be a separate attribute or a parameter. Both options have pros and cons.
But either way, rather than a simple module exists it could be a callback.
There would be some serious restrictions, but you could conditionally add the hooks based on that result.
For example define the hook only if another module does not exist.
No, that was already suggested and addressed by @quietone:
From the MR:
I don't think this is an improvement. Previous comments have pointed out that the capitalization of Contributed is incorrect. That would also apply to 'Core'. This also removes distribution, which should remain. And core does not use the style 'we ask' in any user facing text so this should not as well. The exchange of 'security vulnerability' for a list of items is limiting and I would rather that the more open to interpretation phrase remain.
We are also linking to the resource that the one you linked to links to so it's more direct.
I think it's ok to restore status since this question was addressed already.
quietone → credited nicxvan → .
wim leers → credited nicxvan → .
Thank you for the reviews! The latest round of comments should all be addressed. I left a few open that still need confirmation that the answers are acceptable.
I think quietone addressed your suggestion, thank you!
I addressed all feedback I think.
I think this still needs to be done.
It still gets me occasionally.
Thank you so much! I will review these, kint should have the fix too in it's latest release I think.
Kint is another module that triggered this issue.
Can you do me a favor and go through all of the modules that you have enabled and check the .module file and identify any that have code outside of functions in that file.
Also identify any that implement hook_module_implements_alter, hook_hook_info and maybe hook_requirements? The last one is in the .install files for the modules.
I think this is ready for review again, we've added tests and addressed most of the feedback.
I also added the deprecation.
Beyond review it might be worth identifying a few more things to convert.
Ok this needs some additional tests, but we are in a good spot for another review.
Thanks! I think those notes came after RTBC and I missed them.
I think I've address their comments and yours.
Yes, that bug is concerning, the thing that has me hesitating on both is we're adding an extra compiler pass. Then for this one I'm concerned about performance for two reasons.
1. We need to reflect far more, we had thought that symfony was already reflecting and caching it so we dropped that concern, but we don't have the symfony flag set.
2. We are now rechecking on each hook if it's been changed.
I haven't looked at the event dispatcher issue with an eye to this gap, but I don't think it addresses it.
I need to get a better idea on timing, if this can go into 11.x now that would be good, but I think with 11.2 coming soon we may have to wait a bit, I'm unclear on timing.
Changes from gitlab ci here: https://git.drupalcode.org/project/drupal/-/merge_requests/11841/diffs
More notes from @mstrelan:
https://forum.gitlab.com/t/how-to-share-artifacts-even-on-job-failure/69393
https://docs.gitlab.com/ci/yaml/#artifactswhen
So we might add this:
allow_failure: true
artifacts when:always
When a certain label is applied or it's a draft.
Thanks @nod_
I removed the skip since it doesn't work anyway here is the issue to track: 📌 Consider allowing first stage tests to fail when an MR is in draft. Active
https://www.drupal.org/project/drupal/issues/3515704 📌 Move unit tests to a 'unit tests' stage Active may have some examples to pull from.
Thank you!
I may have to create a meta, but in my mind for now this is the order I think we should work on these:
🐛 HookCollectorPass registers event listeners but they do not follow the required calling convention Active which includes 📌 [pp-1] Explore adding an ImplementationList object for hook implmentations Active
📌 Move hook_implementations_map generation to late in service container compilation to make altering hooks classes more DX friendly Active may not be needed after that change and we may not want to do that at all we need to explore it a bit more.
This issue can likely go after the decoupling from events I would think.
I was concerned about how visible those would be for core committers / reviewers, but that might be a viable way too.
We would need to add a way to target a module for Reorder and Remove because both of those target single implementations.
Can't we move the proceduralcall removal to a followup? It looks like it's not necessary here and is scope creep.
This change feels far more important.
Using the proceduralcall class is not wrong. Internal is just a warning things could change.
We cannot remove it without providing an alternative way to target procedural hooks for ordering.
Great first pass! It's not used all that much, looks like system.install, install.core.inc and a couple of tests. Beyond that there are two spelling failures.
Added a test that swaps out a class and added something to attempt to make the standalone wrappers nicer.
There are several failures, we could possibly revert that, but let's think on it a bit.
I updated the IS as well with some next steps, this could use a pass on comments, and we need to finish tests and tidy up the deprecations.
I will address @dww's comments now.
Haven't had a chance to fully review this but wanted to share https://www.drupalatyourfingertips.com/
Selwyn probably had some good insight!
How do we interact with the procedural calls if that class is gone?
I wonder if we want to do this first: 🐛 HookCollectorPass registers event listeners but they do not follow the required calling convention Active
Turns out core seems to ignore these flags, I just set them to allow failure.
I've added skip rules for a bunch of jobs while we continue to work on architecture, maybe it's worth configuring this so draft MRs don't run these tests by default so that the actual architecture can happen unimpeded, then once the MR is no longer draft we can fix phpcs etc.
I've also added the fix for the fromClass work. I tried running the test locally but for some reason they wouldn't start.
I think this is safe to close.
I think this will conflict with the effort to allow hooks elsewhere, the plan is to only scan the classes i think so we need the method parameter.
I would be curious on which features hux has that the core implementation does not.
More to evaluate if core should consider expanding.
Thank you, I think the point to discuss is that it is acceptable to extend internal code, the only thing is the module author is responibke to test with new releases because core will not provide bc and deprecations.
Rebased, this is ready for review again!
Created the follow up:
📌
[pp-1] Deprecate drupal_verify_install_file
Active
I know this one was bugging you!
I pulled it down and checked to make sure there were no more instances.
Several failures, this definitely won't make it in for 11.2 I think.
Personally I would want to postpone this in the system_requirements conversion. That issue was green and just need to take the new enum into account.
I'll link it later when I fix that issue.
Yeah I don't think they belong on the enum.
I do think it belongs in the same namespace though.
What about Url or Check?
If it's Url we would move just these two probably.
If we call it Check or Checker we could move most of the things remaining in install.inc
nicxvan → created an issue. See original summary → .
nicxvan → created an issue. See original summary → .