Ok I fixed the CS errors.
I also undid the module confirm deprecation because extension confirmation doesn't cover what moduleslistunstableconfirmform needs.
This probably needs some additional work, but I think tests will run.
Also hooks can be numerical. I know hmia docs suggest string, but it's not enforced and there is nothing stopping someone from defining one.
I don't know why coi would enforce it.
The thing is you'd have to update here too: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...
We already had to do this in the hook ordering issue too I think.
Double check this: 🐛 Update hook is incorrectly being registered as a normal hook Active
Oh that is interesting, we definitely ran into this for collection for some enhancements.
Interesting that this was hit in hmia though.
There is an issue to resolve something similar: 📌 Explore inverting checkForProceduralOnlyHooks to prevent them from being added as listeners. Active
I think that issue would break pathauto_update iirc.
@larowlan signed off on the deprecation method on slack so I'm removing framework manager tag. https://drupal.slack.com/archives/C079NQPQUEN/p1740180561937599?thread_t...
From a framework manager POV I'm happy with the approach from a technical perspective. From the POV of should it be one issue or two, it's probably a release manager question. My 2c would be to do it in one issue and sidestep the risk of missing shipping the BC layer.
Leaving the release manager tag for that decision.
I'll update the IS later.
@larowlan signed off on the deprecation method on slack so I'm removing framework manager tag. https://drupal.slack.com/archives/C079NQPQUEN/p1740180561937599?thread_t...
From a framework manager POV I'm happy with the approach from a technical perspective. From the POV of should it be one issue or two, it's probably a release manager question. My 2c would be to do it in one issue and sidestep the risk of missing shipping the BC layer.
Leaving the release manager tag for that decision.
I'll update the IS later.
Ok I took a look through this. I had some high level suggestions to try to make it a bit more readable.
I honestly had more questions than answers, sorry for that. I'll keep an eye on this and try to follow up on the threads.
It seems to me the underlying issue is it is saving it as a non default revision, but it's not creating a new revision, which means you have split data.
A little more detail in steps to reproduce on what to look for might be helpful, e.g. should we set a field and compare two tables?
It has been two weeks, thank you!
nicxvan → created an issue. See original summary → .
nicxvan → created an issue. See original summary → .
I think this might qualify as critical.
That test was a doozy, this is ready for review though.
@smethawee, I don't want to derail this discussion too much more, but you should basically do what was mentioned in #50, but instead of putting the link https://git.drupalcode.org/project/drupal/-/merge_requests/11169.patch copy the content of that page into a file and reference it. Feel free to reach out on slack if you have more questions.
Not 100%sure but i think this is a duplicate of this: https://www.drupal.org/project/drupal/issues/3456176 🐛 10.3 upgrade now missing status-message theme sugestions Active
Ok code sniff is fixed and I fixed all php stan issues for the new hook classes (Except one for bootstrap because bootstrap isn't installed in that case)
This should be ready to go.
Just to be clear, you should download the changes to a file and add that file to your repository in the patched directory.
You should not be using the MR diff directly for patching because anytime someone pushes a change your build will use the new change and you may not have tested it.
Php Stan is failing for covers and one question in the MR.
I did a quick pass all a couple of comments.
One is addressed, one has not.
Test looks good and test only fails.
Needs work for the last comment.
I feel like when it's a multi value field it should always apply to all items.
Please keep the standard issue summary second, it makes it easier to review as the issue progresses.
I think this will be a great addition!
Hi, thanks for working on this!
The test only change passes which means it does not actually cover this fix.
If you are not aware, test only reverts the fix and runs the test to confirm that the fix works.
Test only should fail.
There is an MR, hiding patch files.
This is marked as fixed, but I'm still getting this issue.
The thing is I find this much harder to read, you have to carefully ensure you're on the same line to line up the variable assignments. This gets worse if it's longer.
$magic_map_or_list = [
'varchar_ascii:normal' => 'VARCHAR',
'varchar:normal' => 'VARCHAR',
'char:normal' => 'CHAR',
'text:tiny' => 'TEXT',
'text:small' => 'TEXT',
'text:medium' => 'TEXT',
'text:big' => 'TEXT',
'text:normal' => 'TEXT',
$magic_map_or_list = [
'varchar_ascii:normal' => 'VARCHAR',
'varchar:normal' => 'VARCHAR',
'char:normal' => 'CHAR',
'text:tiny' => 'TEXT',
'text:small' => 'TEXT',
'text:medium' => 'TEXT',
'text:big' => 'TEXT',
'text:normal' => 'TEXT',
I think there are several points leading me toward the second option:
You can immediately see what they are assigned to.
The added burden of handling updates when something changes.
The longer the longest assignment is the further your eyes have to travel whitespace to find the assignment.
If the longest pushes it passed 80 characters, all would be beyond 80 characters.
Diffs for fixes space columns can be messy and hard to read, changing one requires all lines to update rather than just showing the addition or deletion.
I vote for config_entity i think the parallel makes sense.
Thanks!
Baseline is still out of sync, but i think it's a different issue.
https://git.drupalcode.org/project/drupal/-/merge_requests/11198/diffs?c...
Those were not changed in this issue.
They were fixed here:
https://www.drupal.org/project/drupal/issues/3483146
📌
Add string return type to all hook_help implementations
Active
Baseline seems out of sync.
If you would not mind asking core dev their opinion on renaming, no need to bike shed more here. If a core committer agrees I'll rename everything back.
~200 hours to 6 hours or so
wow!
Yeah we need something like this.
Especially with SDCs it's becoming common for me to use set_attribute to the next level below.
So for example this works to set the slot attribute if link is a single value.
button: field_link|set_attribute('slot', 'button__link'),
However if link is multivalue, none of the links get the attribute.
In the parent issue it was suggested to do this:
{% for index,item in my_list_of_renderable_arrays %}
{{ item|add_class('item', 'item--' ~ index) }}
{% endfor %}
but are you excepted to rebuild that as an array? I'd expect it to set the attribute for everything in the list.
I wouldn't be too hasty, there is one person against renaming, and two for it, we can get other opinions.
Also if other's also agree that we shouldn't rename, we can always name everything back to serviceProvider and just tag them.
Putting this back to needs review so we can gather other opinions.
You absolutely can do this with hook_requirements, the plan is to replace the conflicts piece with info yaml or composer.json
Looks great now and I think that's all of the hooks!
Any chance you can compare deleting that many entities with and without this change?
Also memory usage would be helpful.
Also 100M, is that 100 million?
This is so much better!
5 modules were missed:
- File module
- layout discovery
- packagemanager
- Experimentalmoduletestrequirements
- Workspaces
nicxvan → changed the visibility of the branch 3483146-hook-help2 to hidden.
As martinis it's to illustrate they can be renamed.
Yes alter and register are based on the method.
+1 I think it's more readable with just one.
I don't think asymmetrical is really ever more readable in the first place.
Great! I'll keep an eye out!
Are we willing to accept the technically out of scope changes? Or should this become a meta?
The changes need to happen for the return types so I'd be inclined to keep this issue, but I don't want to rebase if it needs more discussion.
Looks great now thanks!
One issue with the deprecation, if you can apply my suggestion I see no other issues after reviewing this, I can RTBC after this.
I had missed a couple of test updates, here is an updated patch.
I recreated the deprecation branch, rebasing has become painful so here is a patch with the changes only.
I'm going to hide it, but I'm attaching it here for posterity.
Updating credit
Updating credit
Updating Credit
Update credit
Updating credit
Looks good to me again, rebase seems clean.