Couple of more comments sorry I missed it my first pass.
The move to the Theme namespace is consistent and the services entry looks right!
nicxvan → created an issue. See original summary → .
nicxvan → created an issue. See original summary → .
nicxvan → created an issue. See original summary → .
nicxvan → created an issue. See original summary → .
That might make sense actually!
Thanks everyone, I'm going to close this as works as designed!
Any chance we can get the pipeline fixed for this issue?
I'm going to close this since there is a possible solution that people can find with google and other guides with much better.
I've added a link to the guide in the readme.
Proposed resolution:
Add link to guide in README
I'm going to close this since we have seen this working.
If you can reproduce this still @griz feel free to reopen this with steps to reproduce.
Do we want to add this in?
Sorry I'm going to close this as won't fix, I had a chat with @q0rban to confirm my thinking, tugboat will not provide anything for this particular project.
All closed issues should provide credit now, thanks for thinking of this project!
Thanks! Merged!
This looks right to me, I'm not sure how to write a test for it.
Any ideas without adding a dev dependency for views_field_views?
It is postponed for the reason given in 7.
We need steps to reproduce so that we can test and confirm this fixes the issue.
If we can add a small section to the readme with a link to the issue you found we can resolve this!
This is ready for review again.
I think this is ready.
We just need the two obsolete CRs deleted and the redirects set up.
I think we need to answer the question on whether we're approaching each phase separately or not.
Actually now that I've looked at this I'm not sure we need this.
InfoParserDynamic is only used by two things:
InfoParser which already uses filecache to cache the results.
InstallCommand which is only used for command line installs.
getProfiles is called more than once, but not sure file cache is worth it in that situation.
system_rebuild_module_data has been gone since D9 too.
I think this is ready again!
This needs tests and profiling, but I moved the patch into an MR to see if anything fails outright.
Ok this is almost there! I took screenshots and I reviewed the code thoroughly.
We don't want to output raw html so I changed it to use item_list.
I added both fixes as suggestions, if you want to use them then you can follow this procedure:
1. look at the diff on the MR that gitlab displays.
2. if it looks right to you you can click either apply suggestion, or if there are more than one change you can click add suggestion to batch
3. once you have added all suggestions you want to apply you can click apply Apply suggestions and put a message
PHPCS might fail since it's a multiline change but I did my best to ensure the spacing is correct.
I ran the core/modules/system/tests/src/Functional/Module/DependencyTest.php locally to confirm it still passes.
I also confirmed that it does check the actual dependencies too.
Once these changes are in and it's green I'll mark it RTBC
Ok I've finally had a chance to take new screenshots.
I think the bullet list is far better!
I'm going to do a quick review again of the code before marking this ready.
smustgrave → credited nicxvan → .
Looks great to me thanks!
I think this is much closer than the number of comments I added world normally imply.
A couple are just too make sure i doesn't miss something.
Several are related to persisting storage (I think we want to keep the profile one, but the rest I think should be false)
One potentially complicated related to migrate, but I think it's handled already.
CR I think should include the file name and content and a brief explanation on when you would choose persist (or at least a link to docs on that option)
I have not forgotten this! Still might be a bit before I can do the ux screenshots.
@cmlara can you share your full design concerns somewhere so we can evaluate?
Whether it affects the likelihood of this issue being resolved I cannot say for now but I think it would be informative to read what seems like a deep analysis as I consider issues in general.
Whoops forgot to change status.
This would probably be good for test resource utilization too.
Yes this would be great!
I think that would be a security concern, it would expose the production db password for any site where someone visits before it is installed if settings are pre-configured.
It can also load the installer if something goes really wrong in production which would also expose the password.
All failures are related to unrelated issues right now except one.
Does accessCheck need to be TRUE or FALSE in:
$entity_ids = $this->entityTypeManager->getStorage('webform_access_group')->getQuery()->condition('type', $entity->getTargetBundle())->execute();
For the WebformAccessHooks function public function fieldConfigDelete(EntityInterface $entity) {
Other than that this is ready for review!
We now have parameters for Hook ordering. If we have a stub for the Hook attribute and there is a module that uses:
#[Hook('hook', order: OrderAfter)] and OrderAfter doesn't exist then there is a fatal error.
However if in addition Hook does not exist then nothing happens.
We could look into adding LegacyHook again since I can't see that getting parameters.
Thank you for reminding me! I need to finish this, but I just did a bunch more.
We did this in the original conversion!
@nexusnovaz you can do them individually or in a batch, when there is more than one it asks you to add to a batch then there is an option to apply the full batch.
Doing it manually is ok as well.
Yep, I resolved my threads. They are different hooks but I think it's clear from the context what they are.
The test called them prefix because Registry does. But I don't think I've seen the module name called a prefix other than that. Same for HOOK being called a suffix.
I don't think that's worth holding this up, but there is one other follow up needed for the router test.
Once those are linked and is green this is rtbc as far as I am concerned.
Please link me to the hook order documentation issue when you create it. They are pretty difficult to hold all in your head so a summary of expected orders would be great, just not sure where.
I did manually map them out and confirm them all as part of the hook ordering issue they were added in, but I couldn't figure out where to document it.
The ones I wrote I kept much more simple with only a couple hooks per test, but this was more comprehensive and realistic test coverage.
We can also consolidate template_preprocess_theme_test_registered_by_module.
Here is the follow up for the test coverage: 📌 Confirm there is a test that procedural preprocess functions are executed properly in modules Active
nicxvan → created an issue.
How are these for comments on each?
diff --git a/core/modules/system/tests/modules/module_test_procedural_preprocess/module_test_procedural_preprocess.module b/core/modules/system/tests/modules/module_test_procedural_preprocess/module_test_procedural_preprocess.module
index 1e7665b412d..99f405dbf76 100644
--- a/core/modules/system/tests/modules/module_test_procedural_preprocess/module_test_procedural_preprocess.module
+++ b/core/modules/system/tests/modules/module_test_procedural_preprocess/module_test_procedural_preprocess.module
@@ -7,14 +7,25 @@
declare(strict_types=1);
+/**
+ * Confirms that template_preprocess_HOOK functions are gathered and executed
+ * when there is no initial preprocess key set on hook_theme.
+ */
function template_preprocess_test($arg): mixed {
return $arg;
}
+/**
+ * Confirms that procedural hook_preprocess() hooks are gathered and executed.
+ */
function module_test_procedural_preprocess_preprocess($arg): mixed {
return $arg;
}
+/**
+ * Confirms that procedural hook_preprocess_HOOK() hooks are gathered and
+ * executed.
+ */
function module_test_procedural_preprocess_preprocess_test($arg): mixed {
return $arg;
}
I'll create a follow up to ensure the template preprocess coverage is as expected.
Is there a way to trigger an error on a shallow copy and require a deep copy?
We wouldn't be able to do this bc break in a minor I think and I think we need to deeply consider the dx.
People are used to being able to just duplicate and I think it's clear we're breaking that.
Maybe duplicate is a better verb for the method?
I created code suggestions that can be applied directly in the MR interface.
When we're adding multiple approaches like this it would be helpful to have a brief summary in the issue summary outlining each MR.
The other issue with the tests is helpful too to simplify this.
Thanks!
Yeah I wonder if project browser covers this, it's mentioned in the IS, but I'm not seeing the actual change proposed.
Let's give @bojhan a chance to reply before closing it.
No problem!
Hiding the old screenshots I'll take be ones later unless someone reviews this before me.
12899 and 12901 I think share the same flaw which is that you need to know to use the special copy function where before you could just copy the array.
I think 12907 solves this, but in possibly the worst possible way cause now it copies every single time.
It should pass though and maybe we can improve it now that we see what is going on.
Maybe a deep copy for the form is better and needing to copy the form and render arrays with a special function is ok, but that feels like bad DX.
I have attached a file of all functions remaining.
I only see system_hook_info and views_hook_info.
There are a lot of template_preprocess, but those are ok.
Couple of questions on the mr.
Shouldn't we get a test for this?
I really would recommend moving the functions, it's the expected resolution we planned for in core.
I pulled the views test since it is technically out of scope and views isn't affected.
I think this is ready, I'll open a followup to confirm there is test coverage for views.
I have a client struggling with performance on dev, I've backported this to 10.4 and deployed it and it seems to have helped after the first couple of page views.
apmsooner → credited nicxvan → .
Yeah this issue is 100% core.
Critical because this prevents saving webform fields.