Perth, Australia
Account created on 21 September 2006, over 18 years ago
#

Merge Requests

More

Recent comments

🇦🇺Australia dpi Perth, Australia

How is this possible though? What is the definition and table?

Are you sure something isnt malformed or altered elsewhere?

Issue template needs to be fixed.

🇦🇺Australia dpi Perth, Australia

Working on this...

🇦🇺Australia dpi Perth, Australia

I think the coverage at \Drupal\Tests\Core\Controller\TitleResolverTest is sufficient.

Other than a tonne of implicit coverage.

PageVariantInterface is just passing valid values down from TitleResolver::getTitle

🇦🇺Australia dpi Perth, Australia

Can we look into and potentially close 📌 Add a test for re-parenting of menu links. Postponed: needs info if there is sufficient overlap.

🇦🇺Australia dpi Perth, Australia

Created a autoloading-related issue for ensuring classes are added to the autoloader before container cache is unserialized @ 🐛 Using instances of classes defined by modules in service container parameters causes fatal errors Active

🇦🇺Australia dpi Perth, Australia

I did find 📌 Extension System, Part IV: Properly register all installed extensions/namespaces during container generation Needs work , which is more about comprehensive/unified namespaces for everything. E.g themes are added to autoloader in a crazy place (\Drupal\Core\Extension\ThemeHandler::addTheme). Distinctly different to this issue.

🇦🇺Australia dpi Perth, Australia

Time has passed, new perspectives and such.

Following on a suggestion and discussion @ #3522410-11: Using instances of classes defined by modules in service container parameters causes fatal errors

What if we did the inverse to what is currently proposed here, back to the original intent of the ticket before direction change in #2536610-6: Ensure all modules are autoloaded with PSR-4 only if enabled

Always have all modules available in autoloader.

Then, the concept of "installed" means whether a modules hooks/services/discovery/plugins etc etc is loaded.

🇦🇺Australia dpi Perth, Australia

Converting what should be a Kernel test to a Functional test in order to get different PHP process is working out! 🎉 Still open to ideas if anyone else knows better.

Coverage

Critically, an error is shown when the new classLoaderAddMultiplePsr4 call in !12038 is removed:

Exception: Warning: unserialize(): Class 'Drupal\autoloader_test\AutoloaderTestEnum' not found
Drupal\Component\Serialization\PhpSerialize::decode()() (Line: 21)

When restored, the test pass once again.

NR

🇦🇺Australia dpi Perth, Australia

Thanks for the suggestion @godotislate, however I don't think thats works.

Problem is I need to add an enum or class reference to the container then get a new kernel either by a new PHP process or programmatically _rebooting_ it, so DrupalKernel uses the warm container cache. However since the test is all the same process, PHP already knows the class exists and caches it (AFAICT), so modifying the autoloader after the fact wont work.

Perhaps I will need a web test since I can rely on fresh PHP processes...

🇦🇺Australia dpi Perth, Australia

Pushed !12038

---

I've pushed up a PR which seems to fix the problem.

I'm at a loss for how to test this, as core/tests/bootstrap.php always adds all modules to the autoloader. I've tried a bunch of workarounds but I cant seem to get far. I'd appreciate if anyone can give that a go...

The flow of DrupalKernel has been moved around a bit, and the nearby type handling has been improved to satisfy PHPStan better, for even PHPStan levels core doesn't enforce yet. Especially with how core is passing around this semi-endorsed "container definition array" to various subsystems, for which I've specifically added a @phpstan-type for.

The way the PR currently works is there is a new cache entry in cache.container cache bin with a cache key with a different prefix to the entry with the entire container definition array.

A new ContainerDiscovery class is implemented to hold onto discovery related data. Objects beat arbitrary arrays. And we can extend it later if we need other discovery related things added to it.

The flow of where classLoaderAddMultiplePsr4</code. is called has been changed up. It isnt evident to me why <code>classLoaderAddMultiplePsr4 is currently in attachSynthetic, and it doesnt look like its relevant to that method as state and namespaces are not modified there. Perhaps it was more relevant many refactors ago. So it has been moved out of attachSynthetic, up into initializeContainer, so I can track and prevent doubling up of calls to classLoaderAddMultiplePsr4

Some parts of DrupalKernel have been changed to be more concise especially when a "discovery" edition of a code block is introduced alongside an existing "container definition" block.

🇦🇺Australia dpi Perth, Australia

Merged this simple workaround.

🇦🇺Australia dpi Perth, Australia

I believe there is an issues somewhere..?

It would be nice to have code always-available, and simplify what "install" means. As in an "enabled" module (etc) adds it do discovery and namespaces, and which hooks are executed.

🇦🇺Australia dpi Perth, Australia

@astonvictor irt your activity on other issues, Ill take a look when I'm back from vacation. For now Im sighting things piecemeal.

🇦🇺Australia dpi Perth, Australia

This is necessary to be able to run linting (make lint) where auditfiles is the root project itself.

I'm not sure what issue youre reporting? What problem is this causing?

🇦🇺Australia dpi Perth, Australia

Notably I could tear apart the hook theme regeneration stuff in compiler pass and move it to a service, but Id still need to construct hook_theme default variables by looking thru definitions, then doing reflection stuff. Which is a PITA.

...or nullify all default theme variables

🇦🇺Australia dpi Perth, Australia

This issue might be suitable to also update \Drupal\Component\DependencyInjection\Container::getParameter to allow enum returns.

Method signatures

\Symfony\Component\DependencyInjection\ContainerInterface::getParameter

public function getParameter(string $name): array|bool|string|int|float|\UnitEnum|null;

\Drupal\Component\DependencyInjection\Container::getParameter

public function getParameter($name): array|bool|string|int|float|NULL {

--

And somewhat related, looking to address use of enums in parameters causing unserialisation on warm loads blowing up since the autoloader isnt fully populated, at @ 🐛 Using instances of classes defined by modules in service container parameters causes fatal errors Active

🇦🇺Australia dpi Perth, Australia

The result is similarly catastrophic for _simpler_ data types stored in the container, like enums.

Enums are a valid return type of getParameter() in Symfony, but Drupal has not been updated to support them.

🇦🇺Australia dpi Perth, Australia

New minor as its a soft break to internal functionality.

Notably, our private "common" library uses it via setPintoHookTheme, so will need 0.5.1, and updates to code:

- param type array -> string
- add unserialize before passing to array_keys

🇦🇺Australia dpi Perth, Australia

There shouldn't be any runtime performance penalty, as the result of \pinto_theme is subsequently cached elsewhere on first-use.

🇦🇺Australia dpi Perth, Australia

Any custom code using pinto.internal.hook_theme must also follow our lead, by unserialising. 🤮

🇦🇺Australia dpi Perth, Australia

Critical points:

DrupalKernel

if (empty($this->moduleList) && !$this->containerNeedsRebuild) {
  $container_definition = $this->getCachedContainerDefinition();
}

and further down, where module PSR4 namespaces are added:

$this->attachSynthetic($container);
🇦🇺Australia dpi Perth, Australia

dpi created an issue.

🇦🇺Australia dpi Perth, Australia

Since theres no direct answer to #7, closing this so there is no wasted effort in this issue, compared to the long-lived/discussed/followed issue at Allow components to define asset libraries Needs work .

Please open if differences can be justified.

🇦🇺Australia dpi Perth, Australia
🇦🇺Australia dpi Perth, Australia
🇦🇺Australia dpi Perth, Australia
🇦🇺Australia dpi Perth, Australia

dpi created an issue. See original summary .

🇦🇺Australia dpi Perth, Australia

Please do not spam tags.

🇦🇺Australia dpi Perth, Australia

To prevent notifications getting routed to catch-all channels, like Chat, this should by default error out (exception) if a notification type is not covered by a configured interface -> channel/transport mapping

This should help with scenarios like:

Notification is routing to a Slack recipient.
The mercure transport is enabled (is a chat channel transport)
The mercure transport will accept the notification with slack recipient by default.

We add something that can limit unintended transports.

\Drupal\notifier_chat_channel\Transport\Slack\Recipient\SlackRecipientInterface: chat/slack

IF a message is routed which dosent match a class/iface/abstract etc combo, then throw exception.

This should reduce the number of custom channelrouter services needed, and also help to prevent unintended message routing if for example more transports within a channel are added progressively.

🇦🇺Australia dpi Perth, Australia

The first implementation has been added, for Slack.

Looks like we'll need to do this for each provider, and adapt concepts from each.

Now that we know the required effort, we can do that in separate issues per service.

🇦🇺Australia dpi Perth, Australia

Incl notification and channel router, per README

🇦🇺Australia dpi Perth, Australia

With big codebases, we're doing php-driven autoregistration without services.yml (as core decoupled from sf and is still bikeshedding about that).
Quite some dev dudes i know with big code bases do it like this.

Thanks @geek-merlin, this makes sense, and allows me to approve this issue. I also prefer everything in `src` as a service, relying on private/service removal if unused, per Symfony.

Perhaps you'd be interested in taking a look at a core ticket I created a year ago: Directory based automatic service creation Needs work to help de-bikeshedding core ;)

Something needs to be done about the HuxCompilerPass2 name, and I'll do a deeper review. This will happen when I can get to it next, but isnt a high priority. Tests, docs, updates also

🇦🇺Australia dpi Perth, Australia

dpi created an issue.

🇦🇺Australia dpi Perth, Australia

In the case where multiple chat channels are configured, greedy channels like Mercure (supports() has minimum requirements), ensure filters filter out for example where a message needs to go to Slack and never mercure.

🇦🇺Australia dpi Perth, Australia

The new CanonicalProduct and Slots attr via php extends inheritance should fulfill the requirements here.

With these systems, you can sub in your own twig/js/css, but inherit from slots and a parent object.

Futher, Slots are the recommended way of working, and they dont have a much surface thats hookable.

If these are not a good solution, let me know!

🇦🇺Australia dpi Perth, Australia
🇦🇺Australia dpi Perth, Australia

For #3, for now, changed to exception if missing file, unless the asset is a glob in which no exceptions are thrown if nothing was found.

🇦🇺Australia dpi Perth, Australia

Also finding that adding asset on a List, where not all components have an asset with that asset name, an error is shown.

As a part of this we should be able to validate whether a file truly exists, and add it conditionally.

Maybe consider adding a param to Asset attr that can ignore non existent files, or throw errors on missing files (default).

Of course, otherwise the Asset attr can be added to individual components/enum-case rather than an asset-attr thats echo'ed from the top of an enum.

🇦🇺Australia dpi Perth, Australia

dpi created an issue.

🇦🇺Australia dpi Perth, Australia

We'll do it outside of BCA for now, but may revisit due to need for a flexible weighting system.

🇦🇺Australia dpi Perth, Australia

Theres been discussions of if or how to scan vendor for plugins before, I dont have references on hand.

I think core would want to look at composer autoload namespace and directories and scan them; whereas the change here is a simple list of which classes to include. No directory scans.

For selfish reasons I dont really have time on hand to look through core, as this is very much a side thing of what I'm working on for PNX. I'd prefer a quick merge but otherwise will advise to use a patch in the meantime.

🇦🇺Australia dpi Perth, Australia

container.namespaces includes only Drupal extensions.

The desire for this issue is to include classes defined in vendor.

🇦🇺Australia dpi Perth, Australia

Tests can be arranged if we can agree on implementation/direction.

🇦🇺Australia dpi Perth, Australia

Cred for auto-invoke idea.

🇦🇺Australia dpi Perth, Australia

e67ead44 includes integration for 0.2.0 except for Asset globs, which need more time as I found typical issues related to how Drupal libraries are relative to app root, and the generic local asset attributes, Css and Js, don't know how to find files (unless they are absolute).

Deferred and will be integrated independently, after probably more abstraction work in dpi/pinto lib @ Add support for Asset globs Active

🇦🇺Australia dpi Perth, Australia

dpi created an issue.

🇦🇺Australia dpi Perth, Australia

dpi created an issue.

🇦🇺Australia dpi Perth, Australia

For SMS Framework v4, the topics discussed here can be achieved with https://comms.docs.contrib.social/sms/verification/conditional

🇦🇺Australia dpi Perth, Australia

Project is obsolete now that SMS Framework provides support for services, including Clickatell.

🇦🇺Australia dpi Perth, Australia

Project is obsolete now that SMS Framework provides support for services, including Clickatell.

🇦🇺Australia dpi Perth, Australia

Project is obsolete now that SMS Framework provides support for services, including Clickatell.

Production build 0.71.5 2024