Account created on 12 June 2008, over 16 years ago
#

Merge Requests

More

Recent comments

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

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?

🇦🇺Australia mstrelan

Yep, that will work. Closing this as a duplicate.

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

mstrelan changed the visibility of the branch 3473714-typeerror to hidden.

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

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

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

I think the hook definition should return string|\Stringable|array|NULL but the implementations should return the smallest subset of these that is applicable.

🇦🇺Australia mstrelan

+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.

🇦🇺Australia mstrelan

I re-opened 📌 Add typehints to hook definitions Active because this doesn't yet cover all of it. I've explained in that issue.

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

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?

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

@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.

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

This probably needs to be opt-in. We could also provide an option to activate the user account or not.

🇦🇺Australia 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.

🇦🇺Australia mstrelan

Cross post with @dpi, seems your should voice that concern on the coding standards issue before it's resolved.

🇦🇺Australia mstrelan

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.

🇦🇺Australia 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.

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

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
🇦🇺Australia mstrelan

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

🇦🇺Australia mstrelan

We now have 1.1.x that drops support for Drupal < 10.3, updating IS to reflect that. Marking NW to rebase.

🇦🇺Australia mstrelan

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?

🇦🇺Australia mstrelan

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

🇦🇺Australia mstrelan

Created an MR from the patch in #10, hiding patches. Have left some feedback on the MR, otherwise it seems to work ok.

🇦🇺Australia mstrelan

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

🇦🇺Australia mstrelan

It's after October 2024, reopening this issue.

🇦🇺Australia mstrelan

No worries are you able to update the test coverage?

🇦🇺Australia mstrelan

I believe this is a core limitation, you'll get an AmbiguousBundleClassException

🇦🇺Australia mstrelan

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)
🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

@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.

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

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;
🇦🇺Australia mstrelan

There are some NodeInterface constants, is that loaded differently?

🇦🇺Australia mstrelan

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

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

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?

🇦🇺Australia mstrelan

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?

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

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 .

🇦🇺Australia mstrelan

mstrelan changed the visibility of the branch 3494385-gitlab-fail to hidden.

🇦🇺Australia mstrelan

mstrelan changed the visibility of the branch 3494385-block-user to hidden.

Production build 0.71.5 2024