🇩🇪Germany @donquixote

I did wonder why Symfony didn't just globally enable it

I would imagine this is for BC reasons, same as here.
There are probably plenty of existing packages that would break if symfony would enable autowire and autoconfigure by default.

::fromFileContents() has @param tag for $file parameter only because it needs extra clarification for Psalm (non-empty-string). All other parameters in that class have no tags. Also note missing @return tags as the return type is clear from typehint.

Interesting example.
For the $contents parameter, the comment description should explain that, if it is provided, the actual file contents are ignored, but the $file is still passed to FileChangeDetector. Maybe in less words.
The ChangeDetector::changed() could have a doc description saying "TRUE, if the resource has changed since the detector was created.".
Also the classes could have doc descriptions.

In this example the code is actually relatively simple, so we can make sense of it without comments.
And tbh there is a risk that a poorly written description is more misleading than helpful. I am already unhappy with my proposed comment above.
Still, in general I tend towards having doc descriptions on most symbols over not having them.

I think it's ok to only document parameters that need clarification. That's quite typical case when parameter type is extended via psalm/phpstan.

It would look quite alien and unexpected in a Drupal codebase, to have param docs for 3 out of 5 parameters of a method.
Having the complete list of parameters gives a sense of symmetry and completeness.
Also it is easier for an IDE to generate the doc comment, and warn if it is incomplete.

But maybe it is just a question of familiarity.
Do you know of 3rd party code with incomplete parameter docs?
(finding such code is the first step, we might still decide that we don't like it)

Maybe we should create an MR with example doc removals.

It would be redundant if the method had a return type though wouldn't it?

The only native return type could be `*(): static`, which is less specific than `@return $this`.
E.g. `): static` would still be correct on a wither method that returns a clone. `@return $this` must not return a clone, it must return the object itself.

Btw, to see how a lack of documentation looks like, we only need to look at the symfony codebase.
I find this painful to read :)

(ignore the previous comment)

That's not , however , how real world code looks like. For example, in EntityFormInterface you can find:

The `setEntityTypeManager()` example is interesting.
The `@param` doc and its description are redundant, because we already have the native type declaration and the first line of the doc comment.
The `@return $this` is _not_ redundant.
The method doc description looks a bit redundant, but actually, I might want more information:
This method needs to be called before the class can be used.
In fact this method reveals a design flaw, where we don't have automatic dependency injection for this class, and instead there are various places where the method is called explicitly.

Each parameter of a function must be documented with a @param tag unless the type already documents it: take the name of a function parameter. Convert it to CamelCase. If the type is an interface then append Interface after. If the result is the same as the type then the doxygen is optional (see other exceptions below).

Again "doxygen is optional" can be confusing. What exactly do we omit? The description, or the entire `@param` tag, or the entire doc comment?

I think the "take the name of a function parameter..." logic is a placeholder for "The parameter is a service".
Usually, if it is not a service, then we should assume the parameter to play a specific role in the class, which deserves to be described.

Problem/Motivation

This is a followup to #3238192: Allow omitting @var for strictly typed class properties → . That has this code snippet:

  /**
    * Where one can order a soda.
    */
  protected Bar $baz;

That's not , however , how real world code looks like. For example, in EntityFormInterface you can find:

  /**
   * Sets the entity type manager for this form.
   *
   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager
   *   The entity type manager.
   *
   * @return $this
   */
  public function setEntityTypeManager(EntityTypeManagerInterface $entity_type_manager);

The entire doxygen is completely superflous although the proposal would only drop @param. We should strive dropping it in its entirety by typing the return then dropping the return and then finally the text.

Or in Drupal\block_content\Routing\RouteSubscriber::childRoutes you can find

   * @param \Drupal\Core\Entity\EntityTypeInterface $entity_type
   *   The entity type.

This is a waste, completely pointless and it is actually harder to read. Because

EntityTypeInterface $entity_type

already totally tells you everything there is.

Benefits

If we adopted this change, the Drupal Project would benefit by ...

Replace 99% of the current doxygen with typing the properties. Rejoice as the amount of boilerplate collapses to a fifth.

Three supporters required

1. @pfrenssen →
2. @nilsdestoop →
3. @claudiu.cristea →

Proposed changes

Drupal API documentation standards for functions →

Each parameter of a function must be documented with a @param tag (see exceptions below).

Each parameter of a function must be documented with a @param tag unless the type already documents it: take the name of a function parameter. Convert it to CamelCase. If the type is an interface then append Interface after. If the result is the same as the type then the doxygen is optional (see other exceptions below).

@var: Class property data types →

Typed class properties may omit the @var declaration. It is recommended to use typed properties whenever possible.

Typed class properties may omit the @var declaration. It is recommended to use typed properties whenever possible. It is also permissible to completely omit the doxygen if the name of the variable -- append the string Interface as needed -- equals the the type.

Remaining tasks

2. /li>
4. Review by the Coding Standards Committee
5. Coding Standards Committee takes action as required
6. Tagged with 'Needs documentation edits' if Core is not affected
7. Discussed by the Core Committer Committee, if it impacts Drupal Core
8. Documentation updates
   1. Edit all pages
   2. Publish change record
   3. Remove 'Needs documentation edits' tag
9. If applicable, create follow-up issues for PHPCS rules/sniffs changes

For a fuller explanation of these steps see the Coding Standards project page →

I would argue that "omit the doxygen" is unclear terminology.
There is no "doxygen" in php, and even outside php, I don't think "doxygen" means a specific part of the doc comment, but rather the mechanism to generate a documentation from doc comments.
For "omit the doxygen" it is not clear whether we omit the param description, the entire `@param` tag, or the entire doc comment.

So instead we should say "omit the param description" or "omit the param tag" or "omit the entire doc comment".

So do we need to add explicit language that doxygen can only be dropped when every parameter is self-documenting?

Absolutely.
Something like this:
- A function doc must have either _all_ `@param` tags or none of them.
- A param doc description can be omitted if the type is a single class or interface, and the parameter name is a derivative of the type name.
- A param doc tag is "trivial" if it only replicates the native parameter type.
- If all parameters are "trivial", then all of them can be omitted.
- If all parts of the doc comment are trivial, then the entire doc comment can be omitted.

I very much support this issue.
The fact that the base fields are enabled by default is a show-stopper for this module on existing websites.

The solution proposed here seems to work in initial tests.
I have not done in-depth testing yet.
Also, I am not sure about BC impact on sites that already use this module.

And this needs tests.

Wonder if this is ready for review?

I don't remember all the details, but at least setting it to review increases the chance of progress :)

The ->setPublic(TRUE) in ->setAlias() makes this quite difficult.

Actually it is possible by replacing the relevant aliases with a custom class 'ResilientAlias extends Alias', which refuses to be made public.
I will probably publish something which does this.
Anyway I still think the implementation from this issue is quite aggressive..

I just ran into this.
I am trying to use the automatic discovery feature from symfony, to discover services in a module and in some composer packages.
This feature relies on unused services and aliases being removed, which only works if they are private.

The ->setPublic(TRUE) in ->setAlias() makes this quite difficult.

Actually it could be useful to add the revision id or the revision date to the file name of a pdf.

I pushed some what I think are improvements - TBD.
See the individual commit messages.

I should also add tests, but that requires a bit more thinking and searching.

See also 🐛 Wrong usage of trim() in entity_print_entity_view_alter() Active .
If we solve that, it will conflict with the patches here..

donquixote → created an issue.

+   * @param int $revision_id
+   *   The entity revision id.

I think the type needs to be "int|null".
This is what I see a lot in core for parameters with NULL default value.

-  public function viewRedirectDebug($export_type, $entity_type, $entity_id) {
+  public function viewRedirectDebug($export_type, $entity_type, $entity_id, $revision_id = NULL) {
     return $this->redirect('entity_print.view.debug', [
       'export_type' => $export_type,
       'entity_type' => $entity_type,
       'entity_id' => $entity_id,
+      'revision_id' => $revision_id,
     ]);
   }

This seems weird:
The redirect route is only meant for legacy support, so we should not expect this to be called with a revision id.
On the other hand: If we support a revision_id parameter, then we need to conditionally change the route for the redirect to use the revision route.

-        '#access' => $access_manager->checkNamedRoute('entity_print.view', $route_params, NULL, TRUE),
+        '#access' => $access_manager->checkNamedRoute('entity_print.revision.view', $route_params, NULL, TRUE),

With the latest patch we are using access check for the revision page even for the regular non-revision link.
I think it would be cleaner to have two separate paths of control flow.
I am going to prepare an MR.

There is an older discussion in #1539712: [policy, no patch] Coding standards for breaking function calls and language constructs across lines → where we also mention conditions with && and ||.

And for the record, I support:
- No line break before first condition.
- Operator && or || at the beginning of a line.
- Line breaks around the ") {".

if ($long_condition_a
  && $long_condition_b
  && ($long_condition_c1
    || $long_condition_c2
  )
) {
  return 'yes';

I do see the appeal of line break before the first condition, but somehow it feels wrong.. but this could be just a preference familiarity.

In the past I would have proposed something like below, but now I think it is awkward..

if (true
  && $long_condition_a
  && $long_condition_b
) {

@scott_euser
Could we come up with a testing scenario to confirm that this works correctly on a given website?

Sorry for changing the status!

I have not tested, but the change looks reasonable to me.
I assume/hope the render element will contain all the necessary cache metadata, even on empty result.

The non-lazy formatter uses addCacheableDependency($view) yet the addCacheableDependency() method bails as soon as it sees that the object does not implement CacheableDependencyInterface (which views does not).

Ouch. My mistake.
I thought I had seen this somewhere else, but in fact it was not the ViewExecutable that was added, but the View config entity itself.
I see the correct solution seems to be to add the `$render_array` which should contain the '#cache' data.

I think we should do housekeeping in the following situations:
- When the entity view or form display form is submitted.
- As a separate command (e.g. drush) or on a dedicated page to do this kind of clean-up.

I don't think we can rely only on specific events.
E.g. when we change the code of a custom module, no event will fire. Especially if we start with an older database dump that knows nothing about fields that were previously added.

I think we don't need to do anything here, at least not for the scenario that I encountered.

donquixote → created an issue.

So....
AnnotatedClassDiscovery actually uses StaticReflectionParser instead of loading the file directly.
This way it can avoid loading a class where the base class might not be defined.
It then adds the plugin definition, without ever loading the respective class.
The plugin is never used only because the field type 'address' does never occur while 'address' module is not installed.

The 'plugin' module does include the plugin classes for all formatters, because the plugin field does not (and cannot) filter them by field type.

So, not sure if anything needs to be done here.

Actually.... I think I am wrong on this one.
The plugin type is just "FieldFormatter", nothing to do with address module, so there is something else wrong here.

donquixote → created an issue.

Special named classes are only for the "implements alter" because I am just not sure whether it's doable or desirable to fire off an event in the middle of DrupalKernel::compileContainer().

Alright. Still not sure that magic naming is the best solution, but..

Other question, do we want `hook_implements_alter()` to work for non-procedural implementations?
With the current PR this would not be the case.
This means whenever a procedural hook gets converted, any code that targets it with hook_implements_alter() would need to be converted too.

Now that the event dispatcher itself allows any object to be passed in rather than requiring an Event class, I think they count as events again. But as soon as we get to multiple parameters and alters etc. that's no longer true either.

So the main differences are:
- by-reference parameters
- multiple parameters
- return value (and the mechanism to combine the return values)

So... yeah mostly not events then but definitely an extension of them.

So we could say Drupal has an extended events system that supports single-parameter classical event subscribers, but also multiple-parameter dispatching with return etc.
So, symfony event system could be seen as a subset of the extended Drupal event system.

Ofc the term "event" implies that there is an actual event object. Or at least this would be a typical understanding of it.
So do we call it an "extended event system" or rather a "dispatching system"?
(Conceptually, "event" does not necessarily mean the object, it can also just mean a thing that happens)

My previous comment suggested creating an implements alter magic named class for the purpose of, well, implements altering the new style ones.

In Hux you can put `#[Hook('something_alter')]` or `#[Alter('something')]` on any method of a hux-enabled class.
I think this DX is preferable over a magic named class.
(The #[AsEventListener] would also still be better than any kind of magic naming.)

In practice, I like to name these classes after their purpose, and often have one class implement multiple hooks with related functionality, or multiple classes implement the same hook for separate functionality.

Typical class names might be "AssignSourceId" or "CoAuthorNodeAccess" or "HideSomethingField".
The class name does not reveal which hook(s) are used to achieve the result, and we might in fact decide to use a different hook without renaming the class.
(Of course others might have different conventions how to name their classes)

I am happy with the feedback so far, thanks catch and donquixote. I do not see any major pain points raised nor anything which would require adding a lot of code. My goal here is to keep both boilerplate and added core complexity down to an absolute minimum.

My previous attempt at this at 📌 Hux-style hooks, proof of concept Needs work had _a lot_ more code.
Unfortunately I did not have consistent time and attention for it.

The main reason for the complexity, from what I remember,
It was a lot more ambitious in terms of BC, and also tried to cover wishlist items like relative ordering etc.

I like the simpler approach proposed here, but it does mean we lose _some_ functionality and BC, especially if we convert existing hook implementations. There might be more nasty details that I no longer remember atm.

I wouldn't worry much about BC, of course old hooks can't just die from one release to the next but I expect the conversion to be automated and very swift.

I am not worried about the effort of the conversion.
My BC concern is about custom and contrib code which:
- relies on a specific order of execution
- uses hook_module_implements_alter() to add, remove or rearrange functions
- relies on conditionally included files ($module.$group.inc)
- implements a hook on behalf of another module
- (perhaps more stuff which I don't remember)

But, if we aim for 11.x then I think this is fine.

So, perhaps like this:
- In 10.x we introduce a BC-friendly mechanism that allows contrib and custom code to define hooks in the new way.
- In 11.x we convert the core hooks.
- In 12.x we drop the old system (as you already proposed).

I do not know much about phpstan but we have excellent people who do -- but if push comes to shove I can write it with nikic/php-parser in less than a day -- I would find out the event name and the function starting and ending line with it but do the actual conversion with just string operations.

You could have a look at the conversion commits here, https://git.drupalcode.org/project/drupal/-/merge_requests/4347/commits

Could we have a subclass of event dispatcher that has a method (or methods) with the same logic? Then obviously you couldn't call dispatch() on those events, but (once the hook bc layer is dropped) it would eventually be possible to call those methods directly instead of via the module handler.

Or we could keep it as a separate class, which just uses the symfony event dispatcher internally.
This would follow the composition over inheritance idea, and avoid interface creep.

Having one big event dispatcher class and interface which also supports hook-style dispatching would mean that components that need both only need one dependency injected. But is this a good reason? Not sure.

One conceptual / philosophical question is whether we think of these new OOP hooks as events, or as something separate, which just uses the event system to collect subscribers.
I think it matters for DX:
If we advertise it as one system, then developers will have the same expectations for hooks and events. This could lead to a situation of surprise or disappointment.
If we advertise it as separate systems (as it is now), then developers can have separate mental models with separate expectations. In this case we should also keep a separate attribute #[Hook(..)].

A developer only sees the dispatcher methods and the subscribed method (signature and return), and both feel different in hooks vs events, even if internally we use the event system for both.

This looks very interesting!

So essentially we use the events mechanism to manage the hook implementations, but we use ModuleHandler to actually call/invoke them.

This also means you are not supposed to invoke these special "hook" events via the event system.
At least those hooks that have return values or that receive more than one parameter.

This is probably fine.
But do we want to "pollute" the event list with pseudo-events that cannot really be used as such?
Or should we have a separate instance of the event system for hooks?

I can't really think of any gotchas here except for hook_module_implements_alter() - but that would still work, implementations might just have to adjust and to run last you'd need to use the new API but that is fine.

This also means that implementations of hook_module_implements_alter() that _remove_ an implementation will no longer work if that implementation gets converted.

I assume this means that class hooks always run after function hooks? It will potentially mean some hook_module_implements_alter() would need updating as implementations get converted, but also I think that is fine.

This is the same as currently in Hux.

If we start replacing core hook implementations, there will be a BC impact due to the changed order.
Maybe we truly target 11.x, or we accept the damage.
Or we introduce the API but don't convert any existing core hook implementations before 11.0.

An interesting question will be alter hooks in general, especially the combined ones like "form" + "form__somethingform".
We could go for a similar solution, if we don't care to replicate the order of execution from the old system, which is quite complex.
Again, the main BC impact will happen if we try to convert existing implementations.

Adding two more scenarios:
- One that is very easy to reproduce.
- One that is explicitly about base fields.

I notice this also happens with entity base fields.

I had the following scenario:
- I worked on a feature that adds a new custom entity type.
- I add base fields.
- I exported the view and form display.
- Now I add entity bundles and replace the base fields with field UI fields.
- I export the view and form display again.

Expected: Base fields are removed from form and view display.
Actual: Base fields remain in the form and view display, even after re-saving the form or view config.

I found that even the old mechanism would not have worked.
So nothing was made worse in this issue.
Sorry for the noise.

The Router::matchCollection() does not call the access checks.
It only calls $route->getCondition() which seems to provide some additional regex checks, but this does not really help to distinguish bundles.

So this is a dead end: We should not have multiple routes with the same path.

The solution implemented here does not work if we have multiple routes with same path, but for different bundles.

The ParamConverter is only called _after_ the route was already picked in Router::matchCollection().
So it is too late at that point.

Something needs to happen with core, as ThemeManager and friends are hell to work with in 2024.

The ThemeManager is a beast indeed, especially the global state around the "current theme".

But adding preprocess callbacks via hook_theme_registry_alter() is quite transparent.
I have done this a lot in Drupal 7, and the mechanism still is the same.

Is something above worth extracting to its own project? Is it reliable/predictable?

Yes, it seems this could go into a separate project.
It could even be fully independent of Hux, although it would be nice to reuse the discovery mechanism.

It seems quite reliable so far.
One thing I don't like is the `\Drupal::service()` calls.
Perhaps theme preprocess will be changed to support service methods, similar to '#pre_render'.

One tricky part could be the more specialized theme hooks, like `region__content`. I will have to investigate a bit more what happens if we add a `$registry['region__content']['preprocess functions'][] = ...`, if there was not such a hook originally.

I like to put multiple hook implementations (and preprocess) into the same class if all belong to the same "behavior".
So I think I would activate the `Hooks` namespace for preprocess.

The following already works with current hux:

#[\Attribute(\Attribute::TARGET_METHOD)]
class Preprocess {

  /**
   * Constructor.
   *
   * @param string $hook
   *   Name of the theme hook.
   */
  public function __construct(
    public readonly string $hook,
  ) {}

}

/**
 * Base class for classes with theme preprocess callbacks.
 */
abstract class PreprocessBase {

  /**
   * Constructor.
   *
   * @param \Drupal\Component\DependencyInjection\ReverseContainer $reverseContainer
   */
  public function __construct(
    private readonly ReverseContainer $reverseContainer,
  ) {}

  /**
   * Implements hook_theme_registry_alter().
   *
   * @param array<string, array> $registry
   *   Theme registry.
   */
  #[Alter('theme_registry')]
  public function themeRegistryAlter(array &$registry): void {
    $reflection_class = new \ReflectionClass(static::class);
    $methods = $reflection_class->getMethods(\ReflectionMethod::IS_PUBLIC);
    foreach ($methods as $method) {
      $attributes = $method->getAttributes(Preprocess::class);
      if ($attributes === []) {
        continue;
      }
      if ($method->isStatic()) {
        $preprocess_callback = [static::class, $method->getName()];
      }
      else {
        $service_id = $this->reverseContainer->getId($this);
        assert($service_id !== NULL);
        $preprocess_callback = [static::class, $service_id . '->' . $method->getName()];
      }
      foreach ($attributes as $attribute) {
        $attribute_object = $attribute->newInstance();
        assert($attribute_object instanceof Preprocess);
        $hook = $attribute_object->hook;
        if (!isset($registry[$hook])) {
          continue;
        }
        $registry[$hook]['preprocess functions'][] = $preprocess_callback;
      }
    }
  }

  /**
   * Invokes a preprocess method on a service object.
   *
   * @param string $name
   *   Synthesized method name, contains the service id and actual method name.
   * @param array $arguments
   *   Arguments to pass to the actual method.
   */
  public static function __callStatic(string $name, array $arguments): void {
    [$service_id, $method_name] = explode('->', $name);
    $service = \Drupal::service($service_id);
    call_user_func_array([$service, $method_name], $arguments);
  }

}

class MyPreprocessHooks extends PreprocessBase {

  /**
   * Theme preprocess for 'block'.
   */
  #[Preprocess('block')]
  public function preprocessBlock(array &$variables): void {...}

}

For a proper solution we would not rely on ReverseContainer and inheritance, instead this would work out of the box for all classes with hooks.

Could you implement hook_preprocess in hux itself (as in hux_preprocess) and then have that call the manager?

There is a nicer way to implement preprocess:
Use hook_theme_registry_alter() to register the preprocess callbacks.

Unfortunately as it is now these functions cannot be methods on services.
From ThemeManager::render():

      foreach ($info['preprocess functions'] as $preprocessor_function) {
        if (is_callable($preprocessor_function)) {
          call_user_func_array($preprocessor_function, [&$variables, $hook, $info]);
        }
      }

But we could use tricks with __callStatic().
https://3v4l.org/SlUU8
https://3v4l.org/kPUuQ
https://3v4l.org/JBk1u

class MyService {
    function preprocess(array &$variables): void {
        $variables['x'] = 'X';
    }
}

class Drupal {
    static function service($id): object {
        return new MyService();
    }
}

class ServiceCallback {
    static function __callStatic(string $f, array $args) {
        ['service' => $service_id, 'method' => $method] = unserialize($f);
        $service = \Drupal::service($service_id);
        $service->$method(...$args);
    }
}

$variables = [];

$callback = ['ServiceCallback', serialize(['service' => 'myservice', 'method' => 'preprocess'])];
call_user_func_array($callback, [&$variables]);

assert($variables === ['x' => 'X']);

donquixote → created an issue.

Sorry, cross post :)

Where does .nvmrc come from to begin with? I am too lazy to look myself now.

When generating a sub-theme, it puts a `.nvmrc` file in the root dir of that sub-theme.
This is actually a copy of `radix/src/kits/radix_starterkit/.nvmrc`.
All it contains is one line saying "lts/iron". I imagine this just means get the latest version with long-term support. But I have no idea.
Effectively it will switch to node 20.x and npm 10.x.

See https://github.com/ddev/ddev/issues/6013
Also this discord convo: https://discord.com/channels/664580571770388500/1199750600653549598/1199...

The problem seems to occur when multiple node versions are installed with nvm in the container.

I found two solutions following these threads:
- Use system node by calling `ddev nvm alias default system`. As long as system node is used, the `nodejs_version` setting from .ddev/config.yml works.
- Call `ddev nvm alias default 20.11.1` instead of `ddev nvm use 20`.

I like the first option more.
But it means at that point we are not really using `nvm` anymore, and we are not using the `.nvmrc` file from the radix sub-theme.

Do these links help?

Not really :)
I also looked into various discussions in the ddev issue queue but still stuck.

The problem is this:
- "nvm use" sets the node and npm version in the host system, so it is quite useless.
- "ddev nvm use" sets the node and npm version in the container, but only for the duration of one terminal session - which is just that one command. So the next "ddev npm" gets the old version.
- node_version in .ddev/config.yml seems to do nothing. And even if it does, it means the version would be defined by the root ddev config, not by the .nvmrc in the theme directory.

(btw we can take this to slack, but I don't find you there)

Related: https://github.com/ddev/ddev/issues/3747#issuecomment-1471889362

Actually, you don't have to do this at all any more. These days the ddev npm command has
## HostWorkingDir: true

which means that it automatically executes in the same relative directory you're in on the host.

The first command does not use ddev but the second and all subsequent npm commands do.

How can `nvm use` do its job correctly if run in the host system?
I get this:

~/projects/ar/ar10/web/themes/custom/ar_radix $ nvm use
Found '/home/lemonhead/projects/ar/ar10/web/themes/custom/ar_radix/.nvmrc' with version <lts/iron>
Now using node v20.11.1 (npm v10.2.4)
~/projects/ar/ar10/web/themes/custom/ar_radix $ npm --version
10.2.4
~/projects/ar/ar10/web/themes/custom/ar_radix $ ddev npm --version
4.2.0

So I have the wrong version of npm in the container.

And then on npm install:

~/projects/ar/ar10/web/themes/custom/ar_radix $ ddev npm install
[...]
npm ERR! notsup Unsupported engine for ar_radix@: wanted: {"npm":">=6.0","node":">=16.0","yarn":">=1.6"} (current: {"node":"7.10.1","npm":"4.2.0"})
[..]
Failed to run npm install: exit status 1

So I guess this works if you already had the correct npm version in the container..

(and btw I like npm ci over npm install, see https://stackoverflow.com/questions/52499617/what-is-the-difference-betw...)

nvm use

You would have to run this within the container, right?
So ddev nvm use?

so using ddev npm run watch to live-sync code changes or ddev composer up to update dendencies both work very well

How do you make it run in the subdirectory where the theme is located?

Is there any argument for "use" vs in-place backslash?
Personally I think I prefer in-place backslash, but "use" seems more common in composer packages.

Clearly in-place backslash is more git-friendly, that is, it reduces merge conflicts from changes in unrelated places.

(In my day to day work I currently don't use either of the two methods, because I would have to convince my colleagues first.)

donquixote → created an issue.

donquixote → created an issue.

donquixote → created an issue.

I created a bunch of commits that improve tiny parts of the old hook.
But these are only meant as a learning exercise.

In the end what matters is the new hook.

donquixote → created an issue.

donquixote → created an issue.

Yes this is related to the core issue.

But we can also do something in this module directly.
We can add an option to append the entity id in parentheses every time, or whenever the title contains parentheses.
So we will have "Dog (female) (123)".

This still needs tests..

donquixote → created an issue.

Hello @swentel
I want to point out that the added contexts have an effect on which block visibility plugins are available in the block UI, for _all_ blocks.

This could have nasty effects if:

• User installs an earlier version of ds which adds a bunch of contexts.
• User configures block visibility based on bundles of an entity type that would otherwise not be available for block visibility configuration. E.g. media.
• Use upgrades ds to a version which no longer adds these contexts, or adds different contexts.
• The block visibility config does not work anymore (?).

Actually I have not really tried how bad it will be, but it should be considered.

donquixote → created an issue.

No idea where to start though...

We need to decide how the routes / paths should be like, and how to navigate between them.

Currently we have paths like '/admin/appearance/styleguide/' . $theme_name.
The navigation is using a secondary layer of tabs (local task links) below the primary tab "Styleguide".

How would a sub-navigation look like?

Actually, maybe the string return is not so useless after all?
It could depend on the browser.
I only tested with Brave so far.

@himanshu_jhaloya You did a number of pull requests following the proposals already in the issue, but sometimes the original proposal needs to be questioned.

bq. That was drupal 8

We want this to work with Drupal 10 :)
Or maybe support different versions.

Now we still return a string which has no effect.

Hello @himanshu_jhaloya
If the code section is indeed useless, we rather remove all of it.
The "return null" experiment was just to see what would happen.

Does this actually work?
Which version of Drupal are you testing this with?

$(".node-form :input", context).once("node-edit-protection")

Remove jQuery dependency from the once feature →

Note on BC:
Somebody could have custom code that writes to this object.
Such custom code would no longer work.
But I don't think this module has any commitment to support such custom code.

Looks good to me.
I don't know if there are more niche cases we need to test, but I don't see what would go wrong.

Needs maintainer review.
It would be good to know why this code was added in the first place. Probably a leftover.

For this specific issue, I think a good solution would be to use php to add a class to the form, and then in js just pick up that class.

Secondly, the code treats the Drupal.behaviors.* object as a namespace, which is not really how this is normally done.
The common pattern for globally available functions seems to be Drupal.modulename.foo = function () {...}.

It is not wrong, but it is generally good to follow familiar patterns.

I don't think this is the way to do it.

First, the patch contains some unrelated changes that have potentially harmful side effects.

-      $(".node-form :input").each(function() {
+      $("." + contentType + "-form :input", context).each(function() {

The basic idea of filtering for context here is good, but this does not work if context is an element within the `.node-form`.
E.g. if you have an ajaxified element inside the node form, then inputs within the ajax-loaded content will be filtered out.

Put a multiple-value link field in the form. Click "Add another item". Edit the new value. Navigate away.
The edit protection dialog won't show up.

Btw mdn documents a different way of using this event:
https://developer.mozilla.org/en-US/docs/Web/API/Window/beforeunload_event

Instead of a return value, they call event.preventDefault()

donquixote → created an issue.

donquixote → created an issue.

donquixote → created an issue.

donquixote → created an issue.

The proposed code looks more complicated than it needs to be.
The following check should be sufficient:

    if (document.querySelector('[data-ckeditor5-id][data-editor-value-is-changed=true]')) {
      [...] // Changes in ckeditor exist.
    }

Overall the code can be cleaned up like this:

  const unsavedChangesExist = () => {
    if (click) {
      return false;
    }
    if (edit) {
      return true;
    }
    // Check modifications in CKEditor 4 instances.
    if (typeof CKEDITOR !== 'undefined' && CKEDITOR.instances) {
      for (const instance of CKEDITOR.instances) {
        if (instance.checkDirty()) {
          return true;
        }
      }
    }
    // Check modifications in CKEditor 5 instances.
    if (document.querySelector('[data-ckeditor5-id][data-editor-value-is-changed=true]')) {
      return true;
    }
    return false;
  };

  // Handle backbutton, exit etc.
  window.onbeforeunload = function() {
    if (unsavedChangesExist()) {
      // Force the warning dialog.
      return true;
    }
    // Do not show the warning dialog.
    return null;
  }

donquixote → created an issue.

I wanted to give this a try, but I give up.
There are too many places that call field_group_info_groups() which do not have the altered $display object available.
Sad!
It also makes me think that in core itself this hook is not very useful.

donquixote → created an issue.

Client-side libraries that enhance the UX for autocomplete (tags style) widgets.
(Chosen or slim_select already do this for select lists, but not yet for autocomplete, afaik)

https://www.drupal.org/project/autocomplete_deluxe →
This module does it.
It acts like an equivalent to chosen or slim_select for autocomplete.

A strategy could be like this:

• A widget type that dynamically switches between select list and autocomplete, depending on the number of available options.
• Optimized code to build a huge options list without loading all entities into memory at once.
  Part of this can be an entity iterator, another part can be caching of entity labels, and caching of complete option lists. 🐛 Options Select Widget: High Memory usage Active ,
  📌 Add an entity iterator to load entities in chunks Needs work , #1199866: Add an in-memory LRU cache → .
  This can be useful if we have something like < 500 options.
  We could even load the list as a separate request, so it can be cached separately _and_ does not impact initial load time.
• Client-side libraries that enhance the UX for autocomplete (tags style) widgets.
  (Chosen or slim_select already do this for select lists, but not yet for autocomplete, afaik)

I see discussion around point 3 (client-side libraries) in this issue, but it is not really 100% focused on that.
Perhaps we should open individual issues for the different aspects?

There are two issues with long select lists:

• Server-side performance and memory impact of building the option list, especially if we need to load all the entities. See 🐛 Options Select Widget: High Memory usage Active .
• Network and client-side impact.

In my experience, we run into the first problem much earlier than the second problem.

And it's also the reason why the autocomplete widget exists :)

I was going to switch a lot of widgets to autocomplete on a big site I work on.
Unfortunately this can come with some usability limitations.

Currently we use https://www.drupal.org/project/slim_select → which works for select widgets but not autocomplete widgets, because it does all the magic on client side.
I can imagine a similar widget could be created for autocomplete, but I have not found one yet.
Also, having the full option list on client side will make the UI more snappy as it avoids the need for ajax requests.

There is a middle ground of ~250 options, where we want a select list with optimized loading, rather than repeated autocomplete ajax requests. E.g. a country dropdown could have this order of magnitude of options.

Actually I _can_ reopen it!
The only case when we should close it is if we decide to open a new issue, or find one.

This is by design, in order to get the translated label of an entity we need to load the full entity object. And it's also the reason why the autocomplete widget exists :)

It is generally not needed to load all entities into memory at once.
To get a translated title while saving memory, we can/could/should load and "forget" the entities one by one.

We could also cache individual translated entity labels, and complete option lists.
(This needs cache metadata to deliver different filtered option lists based on user permissions.)

I don't see a "reopen" button here, but it would be appropriate.

donquixote → created an issue.

greggles → credited donquixote → .

donquixote → created an issue.

donquixote → made their first commit to this issue’s fork.

    if (is_a($source_plugin, D7Vocabulary::class)
      && !is_a($source_plugin, D7VocabularyTranslation::class)
      && !is_a($source_plugin, D7LanguageContentSettingsTaxonomyVocabulary::class)) {
      if (isset($migration['process']['vid'])) {
        $process[] = $migrations[$migration_id]['process']['vid'];
        $migrations[$migration_id]['process']['vid'] = $merge_forum_vocabulary($process);
      }
    }

This code can lead to problems.

In core 'd7_taxonomy_vocabulary', $migrations[$migration_id]['process']['vid'] is a single process plugin, so all is good.

But with a custom migration, as with migrate_plus, $migrations[$migration_id]['process']['vid'] is already an array of process plugins. The $process[] = .. assignment will result in a nesting level that is one level too deep.

As a result, I get "Undefined array key "plugin" Migration.php:727" for the upgrade_d7_taxonomy_vocabulary migration.

In fact, the upgrade_d7_taxonomy_vocabulary generated with migrate_plus contains has the 'forum_vocabulary' processor in 'vid'.
So adding it again would result in duplication.

As a workaround, I can edit the custom migration, by copying the 'vid' process setup from 'd7_taxonomy_vocabulary'.
Still, this should be fixed.

We can set the title directly in a controller like:

return [
  '#title' => $migration->label(),
  'content' => $build,
];

But this does not work that easily for entity form pages where we don't own the controller.

There should be a distinct route setting for "breadcrumb title callback" or perhaps "navigation title callback".