Merge Requests

More

Recent comments

🇺🇸United States nicxvan

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.

🇺🇸United States nicxvan

I'm not sure what other review this needs it's essentially a one for one move to the new classes.

🇺🇸United States nicxvan

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.

🇺🇸United States nicxvan

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!

🇺🇸United States nicxvan

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.

🇺🇸United States nicxvan

Thanks! That isn't quite what I meant, that copies all values from another instance, I want to copy just individual subfields.

🇺🇸United States nicxvan

I can confirm this is fixed in dev.

🇺🇸United States nicxvan

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.

🇺🇸United States nicxvan

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.

🇺🇸United States nicxvan

nicxvan changed the visibility of the branch 3525331-simplify-widget to hidden.

🇺🇸United States nicxvan

Yep generally the older one wins, but the one worth work should move forward.

I closed the other.

🇺🇸United States nicxvan

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.

🇺🇸United States nicxvan

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.

🇺🇸United States nicxvan

nicxvan created an issue.

🇺🇸United States 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.

🇺🇸United States nicxvan

I think quietone addressed your suggestion, thank you!

🇺🇸United States nicxvan

I think this still needs to be done.

It still gets me occasionally.

🇺🇸United States nicxvan

Thank you so much! I will review these, kint should have the fix too in it's latest release I think.

🇺🇸United States nicxvan

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.

🇺🇸United States nicxvan

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.

🇺🇸United States nicxvan

Ok this needs some additional tests, but we are in a good spot for another review.

🇺🇸United States nicxvan

Thanks! I think those notes came after RTBC and I missed them.
I think I've address their comments and yours.

🇺🇸United States nicxvan

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.

🇺🇸United States nicxvan

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.

🇺🇸United States nicxvan

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

🇺🇸United States nicxvan

https://www.drupal.org/project/drupal/issues/3515704 📌 Move unit tests to a 'unit tests' stage Active may have some examples to pull from.

🇺🇸United States nicxvan

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.

🇺🇸United States nicxvan

I was concerned about how visible those would be for core committers / reviewers, but that might be a viable way too.

🇺🇸United States nicxvan

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.

🇺🇸United States nicxvan

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.

🇺🇸United States nicxvan

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.

🇺🇸United States nicxvan

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.

🇺🇸United States nicxvan

Haven't had a chance to fully review this but wanted to share https://www.drupalatyourfingertips.com/

Selwyn probably had some good insight!

🇺🇸United States nicxvan

How do we interact with the procedural calls if that class is gone?

🇺🇸United States nicxvan

Turns out core seems to ignore these flags, I just set them to allow failure.

🇺🇸United States nicxvan

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.

🇺🇸United States nicxvan

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.

🇺🇸United States nicxvan

I would be curious on which features hux has that the core implementation does not.

More to evaluate if core should consider expanding.

🇺🇸United States nicxvan

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.

🇺🇸United States nicxvan

Rebased, this is ready for review again!
Created the follow up: 📌 [pp-1] Deprecate drupal_verify_install_file Active

🇺🇸United States nicxvan

I know this one was bugging you!

I pulled it down and checked to make sure there were no more instances.

🇺🇸United States nicxvan

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.

🇺🇸United States nicxvan

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

Production build 0.71.5 2024