I agree with #11, we're not depending on this to enable a phpcs rule so if there happens to be any that are missed they can go in their own issue.
It's not possible to do return only because some implementations do not return anything. They need to at least be refactored to return NULL. Fwiw I initially recommended adding a return to the end of these functions but the feedback was that it was confusing and there was a preference to initialise the return value at the start and return at the end.
So, should we split this to just fix the ones that always return something and address the others separately? Or just leave it baselined and wait till hook_help is replaced entirely by help topics?
pfrilling → credited mstrelan → .
Yep, that will work. Closing this as a duplicate.
Both the label and description are usually FormattableMarkup objects, or sometimes strings. Let's leave the library to require strings and do the casting on the Drupal side.
mstrelan → changed the visibility of the branch 3473714-typeerror to hidden.
Moved to Prometheus Exporter queue since any Drupal solution would apply to more than just Prometheus Webform, and any library solution can be discussed there. Wonder what that will do with the existing issue fork.
I'm not sure about #4. PNX\Prometheus\Metric
can't do anything special with Stringable objects we pass to it, so it makes sense to limit it to only primitive strings. What if instead we cast to string in \Drupal\prometheus_exporter\Plugin\BaseMetricsCollector::getDescription
? That would mean the existing return type in \Drupal\prometheus_exporter\Plugin\MetricsCollectorInterface::getDescription
is valid and we don't have to mess around with types in multiple places.
Postponing on 📌 Add void return to @ingroup entity_crud hook implementations Active and 📌 Add return type to remaining hook implementations Active . Added steps to reproduce. Added a draft MR with all the alter hooks updated.
I am not sure we could do this until Drupal 10 EOL because it feels like it would make cherry-picking backports from 11 to 10 much more difficult. Unless it is possible to polyfill the attribute (even if it does nothing) in PHP 8.1?
Yes symfony polyfill does this
I thought I'd chime in to clarify why I'm undecided on requiring comments to be formal "docblock" style. I think that enum case names should generally be self documenting, but in the case that they need further explanation they probably should have a thorough, well thought out comment. But perhaps there are cases where a single-line comment is fine (e.g. the existing FileExists comments). Ultimately I think it's probably better not to be prescriptive about this, so changing my vote to -1.
Re-based and re-baselined
I looked a little closer and it's not necessarily when the library is attached to the page, it's simply when the library exists in the libraries.yml file that it gets added to the CSP. Perhaps the library could be dynamically defined in hook_library_info_build
instead. It would be better not to have three separate libraries and only a subset of them that actually work.
mstrelan → created an issue.
There's also \Drupal\Core\Plugin\DefaultPluginManager::$cacheTags
and \Drupal\Tests\Core\Render\TestCacheableDependency::$tags
that are simple arrays.
For the record, the reason I wanted to test with a higher phpstan level is to determine if there is somewhere in core that is using something other than a list. But upon further consideration it probably doesn't matter, because the changes were initially made to satisfy phpunit, and these changes are to satisfy phpstan.
Most likely it will be contrib and custom projects that are affected here, as they're more likely to run on higher levels of phpstan.
I set phpstan to level 3 and generated the baseline. Then applied this patch and re-generated again. There was only one difference and it was just rearranging the wording of one violation.
Then I repeated the same steps on level 8 to see if we're not creating and masking more issues.
The diff mostly shows places where we were calling Cache::mergeTags
with an array instead of a list, mostly due to RefinableCacheableDependencyTrait::addCacheTags
.
It does, however, show that we probably also need to update the param doc for CacheableMetadata::setCacheTags
, as that allows setting the property to an array that is not a list.
It also doesn't like \Drupal\Core\Entity\EntityBase::getCacheTags
returning ::getCacheTagsToInvalidate
as once again that is not necessarily a list.
There are several more problems in various places, but I haven't combed through them all. Attaching the diff for perusal.
Adding related coding standards issue since it will inevitably come up. Don't think we should wait for it though, especially since we already use this in \Drupal\Core\Cache\Cache::mergeTags
.
Added a CR
I think we need level 3 before we see phpstan issues.
Level 2: https://phpstan.org/r/98f1fbe4-0a95-4f4e-bd29-d8869033c9ea
Level 3: https://phpstan.org/r/a58dfaa7-b313-43ce-8bff-8c60afde83e3
That's a tricky one to answer. The only place this module is calling createUser is in OpenIDConnect::completeAuthorization
, and that already has checks to see if the user already exists. In that case it would be fine for the exception to bubble up. The doc block for \Drupal\externalauth\ExternalAuthInterface::register
doesn't mention that it throws that exception either, so in theory any concrete class implementing that interface doesn't need to throw that exception either. So I think bubbling up would be fine.
I think the hook definition should return string|\Stringable|array|NULL
but the implementations should return the smallest subset of these that is applicable.
+1 to allowing optional comments.
+1 to allowing those comments to be formatted as a formal docblock
+ 1 to allowing optional new lines where appropriate for legibility.
-1 to requiring comments.
0 to requiring comments to be formal "docblock" style.
-1 to requiring newlines.
mstrelan → created an issue.
I re-opened 📌 Add typehints to hook definitions Active because this doesn't yet cover all of it. I've explained in that issue.
Re-opening and moving to a child of 📌 [META] Add return types to hook implementations Active because there are a lot of hook definitions (in .api.php files) that were not updated in some of the wildcard issues, like all info hooks. I think we can use this to identify and fix any leftovers.
mstrelan → created an issue.
Ah, we can just use the path_processor_manager
service that will run for all path processors. Have put up an MR. Obviously needs test coverage. May need to consider BC concerns. May need config to opt in or out.
Didn't mean to change the status. It's probably not as simple as the proposed resolution. We need to do something like this:
$currentPath = \Drupal::service('path_processor_subpathauto')->processInbound($currentPath, $request);
But obviously that doesn't make sense to do here, so I'm wondering if there is a generic way to handle this?
mstrelan → created an issue.
mstrelan → created an issue.
I agree with @Berdir's comments about the value object, but in the interest of removing the .module file I think what you have now is the simplest. We should open a follow up to move to a value object.
@smustgrave a lot of the time where we had @return string
we're actually getting a MarkupInterface (e.g. TranslatableMarkup) but since we don't have strict types in runtime code yet it gets coerced to a string. Once example is twig_render_template
. We could be more prescriptive and use MarkupInterface instead of Stringable, but I think we should allow both. Besides, implementations can return whatever they want, the hooks in .api.php files are just examples.
I think we can drop the number 7 from the title. No one is using PHP 7 any more, and PHP has evolved since return types were introduced. Adding my name as a supporter.
This probably needs to be opt-in. We could also provide an option to activate the user account or not.
Updating status
smustgrave → credited mstrelan → .
The proposed text talks about a specific scenario which is already a non-strict comparison. So disallowing the short ternary syntax does not improve this. It may encourage people to learn more as to why it's disallowed, and refactoring to use a strict comparison, but in most cases people would probably just expand it out to the longer syntax.
Cross post with @dpi, seems your should voice that concern on the coding standards issue before it's resolved.
Well there has been an MR for almost 12 months, so this is Needs Work, as it needs a reroll. Adding appropriate tags to hopefully encourage someone else to get this green.
quietone → credited mstrelan → .
Updated the following:
Drupal\search_langcode_test\Hook\SearchLangcodeTestHooks::searchPreprocess
Drupal\comment\Hook\CommentHooks::nodeUpdateIndex
claro_views_pre_render
contextual_preprocess
deprecation_test_deprecated_hook
module_test_altered_test_hook
module_test_modules_installed
module_test_modules_uninstalled
module_test_test_hook
nyan_cat_extension
nyan_cat_render_template
test_subtheme_views_post_render
test_subtheme_views_pre_render
twig_extension
twig_render_template
I'm sure someone with better grep-fu will find some I've missed.
mstrelan → created an issue.
I think this also needs to update layout_builder_test_node_view
, and the doc block should reference hook_ENTITY_TYPE_view
not hook_entity_node_view
.
Looks like I missed some.
Fixed and added to the IS:
- node_theme_suggestions_node
- big_pipe_theme_suggestions_big_pipe_interface_preview
- views_ui_theme_suggestions_views_ui_view_preview_section
These are alter hooks that return void and have not been updated here.
- MY_MODULE_theme_suggestions_alter
- hook_theme_suggestions_alter
- MY_MODULE_theme_suggestions_node_alter
- hook_theme_suggestions_HOOK_alter
Tried to rebase but too many issues, merged in 1.1.x and resolve the conflict on composer.json
Postponing on ✨ Let GDToolkit support AVIF image format Needs work anyway
mstrelan → created an issue.
mstrelan → created an issue.
We now have 1.1.x that drops support for Drupal < 10.3, updating IS to reflect that. Marking NW to rebase.
Tested this with Drupal 11, with ✨ Let GDToolkit support AVIF image format Needs work and ✨ Add support for GD Needs review and can confirm it's working as expected.
I think we need to add a version constraint to composer.json since we have breaking changes for Drupal < 10.3 so sites on 10.2 can't accidentally update to this. Or does the composer facade handle this automatically from the .info file?
Since we have nonce support now this should no longer be postponed. Please change it back if I'm wrong. I'm interested to see if we can add the nonce to all external scripts that are loaded by libraries and therefore allow strict-dynamic
Created an MR from the patch in #10, hiding patches. Have left some feedback on the MR, otherwise it seems to work ok.
mstrelan → made their first commit to this issue’s fork.
Fixed the two methods mentioned in #5, not sure how they were missed before.
Re #4:
Well other than hook help still
I mean those that don't already have an issue
It's after October 2024, reopening this issue.
No worries are you able to update the test coverage?
I believe this is a core limitation, you'll get an AmbiguousBundleClassException
I don't see this as a coding standard issue. I also don't see how this violates existing coding standards. The only confusion I can see is that we usually write FALSE
, but that's when we're referring to it as a value, not as a data type. The
data type indicator →
notes in the coding standards clarify this.
For the PHP built-in types, use the following names:
- ...
- false (NOT "FALSE", see bool)
I've updated the MR so we're not ever setting decimal weights in the original form. Instead we use a decimal value in an intermediary array, sort that array, and then set the weights in the original array. The new weights should be sequential starting from 0.
mstrelan → created an issue.
mstrelan → created an issue.
@spokje I didn't look at what they were. It wouldn't have been baseline additions, because these issues are only reported once the config is changed as per the MR here.
Rebased and updated those in #4, #5 and #8.
I rebased the MR hoping to see the number of errors go down since 📌 Fix more var and param docs identified by phpstan Active , but it's actually gone up from 381 to 436.
I'm failing to understand how they wouldn't be available. Can't contrib do this?
<?php
use Drupal\whatever\SoneInterface;
$foo = SomeInterface::MY_CONSTANT;
There are some NodeInterface constants, is that loaded differently?
IMHO for a quick fix they should stay as constants instead of enum. Enum is better when a variable can be one of the enum case values, but these are only ever array keys as far as i know
Yeah for the array. So the constants would essentially become object properties. I've been AFK and will continue to be for another week or so, so apologies if I'm talking nonsense, as I'm just glancing at it.
I take it back, since these are used as array keys you need to use the scalar value.
The array structure is a bit weird though, maybe it would make sense to have a value object class instead of associative arrays. Maybe enum doesn't make sense after all. Let's see what others think?
I didn't look too thoroughly, but ideally we can pass around the enum itself rather than the string backed values. Is that possible or are there too many bits to untangle?
Rebased the MR.
The first thread appears to have an open discussion.
I have refactored this as requested.
For the workspace one should we postpone till workspace is refactored?
Don't think so, there is no harm in leaving it in.
I have reviewed MR 10612 and appreciate the updated issue summary. Had a question on the MR but I think it's a non-issue. All looks good to me and pipeline is green.
Answered the question in the MR and rebased
Rerolled
Rerolled
As per slack it's probably not necessary to skip caching when $libraries_to_load
is empty, as there is still a fair bit of logic that can be cached, such as hook invocations and asset optimisation.
This looks great. I stepped through this with the problematic webform mentioned in the IS. At first I thought it was still problematic in that it was still getting and setting the cache when $libraries_to_load
was empty, but then I was relieved to see that $asset_settings
was still merged in regardless of what happened with the cache.
mstrelan → created an issue.
First of all I was going to say we should hash the settings into the cache key, but the settings logic isn't cached.
I don't think we can hash the settings, they contain ajaxTrustedUrl
which has keys like form_action_p_pvdeGsVG5zNF_XLGPTvYSKCf43t8qZYSwcfZl2uzM
and ajax
which has keys like edit-actions-submit--mclJzp_PNkM
. It seems at least the edit-actions-submit
suffix changes on each request.
Some of the settings logic is cached, and the rest of it is not. I will admit I don't really understand what's going on in this function.
I've pushed a second branch which attempts to do zero libraries work if there are no libraries, but also doesn't cache anything. Didn't run tests on that yet, approach could be completely flawed.
Seem to be a few test fails related to ajax, not sure if they are random fails and I don't see a retry button.
The whole function could use a refactor, but that's probably blocked on #2614936: Improve unit test coverage for AssetResolver → .
mstrelan → changed the visibility of the branch 3494385-gitlab-fail to hidden.
mstrelan → changed the visibility of the branch 3494385-block-user to hidden.
mstrelan → created an issue.
Needs tests etc but setting NR to get eyes on it
mstrelan → created an issue.