🇬🇧United Kingdom @joachim

> Also may want to check usage as I don't see anything in core besides a test using this hook. Maybe it's not needed?

It's standard for all plugin managers to have an alter hook for the discovered plugin info.

joachim → created an issue.

> and it turns out the only issue is that the paths weren't prefixed with a preceding slash, so the path was appended to the path of the previous request resulting in 404s

Can you explain a bit more about where this path appending thing happens? Is it something we can maybe fix?
Though OTOH, since the Drupal convention is now that paths should have the initial slash, it's probably a good thing to add that consistently.

I've had a look at your patch on the other issue for converting BlockContentPageViewTest. I'm honestly confused by what this failing test is doing anyway:

  public function testPageEdit(): void {
    $block = $this->createBlockContent();

    // Attempt to view the block.
    $this->drupalGet('block-content/' . $block->id());

    // Ensure user was able to view the block.
    $this->assertSession()->statusCodeEquals(200);
    $this->drupalGet('/block-content-test');
    $this->assertSession()->pageTextContains('This block is broken or missing. You may be missing content or you might need to install the original module.');
  }

Why would the block report as being broken? What's causing it to be broken? This test needs to explain what it is doing!

@mstrelan could you try adding this to the top of AssertContentTrait::getTextContent()?

    // If this test uses \Drupal\Tests\HttpKernelUiHelperTrait to make requests,
    // then get the content from the Mink browser.
    if (isset($this->mink)) {
      return $this->getSession()->getPage()->getText();
    }

Thanks for having a look!

AssertContentTrait wasn't change by this MR.

I'm not sure whether to:

a. call AssertContentTrait::setRawContent in drupalGet()
b. change getTextContent() to use Mink, since that's now available? And then we can later on deprecate getTextContent().

> but I couldn't get the foobar_gorilla block to load in ::testPageEdit

Could you push your work on that test somewhere so I can have a look at it?

> The first blocker I couldn't easily work around was that the default cache policy for DPC prevents caching if PHP_SAPI is cli

The docs on HttpKernelUiHelperTrait::drupalGet() say that page caching modules won't work. Should that list of warnings be moved from the method docs to the trait docs, so they're more visible?

Using modules for feature flags feels wrong to me. It's piggy-backing onto an already complex system, which lots of things already talk to.

We'd be adding empty modules, which then all the various discovery services have to waste time checking -- plugins, routes, services, permissions, etc etc.

> - you have to add the configuration schema,

Wouldn't we have one central config for the feature flags, and so only one schema? And they config values are validated by being valid feature flag plugin IDs. (Or we could just cheat and declare the array to be of type 'unknown' :p)

> and an update path to set the default 'off' state of the flag

Wouldn't we have to do that with modules too, to enable them?

Or what about having a system where when updates are run, any discovered feature flag is set to 'on' unless it's explicitly 'off' in config?

> - you have to add a form somewhere so site owners can change the flag. This would either require a central 'feature flags' page or it would be dotted around the UI.

That's just one form.

> - when the feature is eventually enabled by default, it requires another schema change and another update path to remove the configuration.

We'd need that with module too, no?

The other downside of using modules is that it's messy for the admin user. With a separate feature flag config and UI, there is a clear and simple way for a site admin or site developer to answer the question 'Have I got any enabled feature flags?' With modules, how do you find that?

Merged, thanks!

Yes, it's still relevant.

The documentation around the NULL/[] difference for target bundles was updated in another issue about a year ago. IIRC it got fixed, so there should be an explanation in code now.

Tagging as NISU, as only option 2 is currently outlined in the IS.

I think that if 📌 Move source_module and destination_module from Migrate to Migrate Drupal Needs work makes the original proposal possible, we should wait for that, as that has the best DX IMO.

In core's autocomplete.js:

      const options = $.extend(
        { success: sourceCallbackHandler, data: { q: term } },
        autocomplete.ajax,
      );
      $.ajax(this.element.attr('data-autocomplete-path'), options);

We'd need to override autocomplete.ajax to include something like

data: { q: term, *something else here } },

joachim → created an issue.

joachim → created an issue.

joachim → created an issue.

Thanks!

I'm not sure this fixes the original problem, so leaving open

Yeah, module_builder_entity_type_build() expects to find it, because it dynamically adds further templates and needs a base URL.

Ru had some module which was crashing when config entities had a canonical template.

Try changing it to 'edit-form' in module_builder_entity_type_build(), since that has the same path.

Are you using the same module that Ru is which complains about canonical templates?

I can't reproduce your problem.

Yup, this module works well with Facets made with exposed filters.

joachim → created an issue.

Potentially? You'd need your filters to be made with a view.

I considered doing expanding fieldsets on a single overview page, but decided to follow the UI pattern on eBay and Vinted, of using subpages for each filter. I don't think supporting both UI patterns would make sense in a single module.

Breakpoint files are not config! They're YAML plugins.

Putting the whole seriali

> This is because prior to saving layout, the custom block entities exist in a limbo state, where they have no ID and thus cannot be loaded from entity. This breaks the entity load in displayView().

I've run into this problem elsewhere. There are two options:

- bypass the lazy builder if there is no entity ID and render directly. We know that we're not in a main rendering situation, so the performance hit is fine
- don't render anything if there is no entity ID

> Because changing node.settings.use_admin_theme requires a route rebuild

Let's add a comment to say that. Otherwise someone in the future will wonder about that all over again.

joachim → created an issue.

The big problem with converting form/render arrays to objects is BC. There's tons of form building code and form altering code out there which expects arrays, and passes them to PHP native array functions like array_merge() and so on.

If PHP have an Arrayable interface (like its Stringable) and if the array_foo() function accepted that, we'd be fine, but it doesn't.

I've thought of a way we could maybe do this, though it's not very pretty...

1. We pass a render object to form alter, wrapped in a catch{} block for a TypeError.
2. If we catch a TypeError, then it means something in the form alter chain is using array_foo() or something like that.
- 2.1 trigger a deprecation error
- 2.2. Convert the render object to an old-style render array, and pass it into form_alter again.

It would be:
- bad for performance
- you'd only get a deprecation error for the first use of an array_foo() function, and you'd have to clean that up before you saw the next one. Which would make it a PITA for updating your custom code, because you'd be seeing all the contrib ones you can't do anything about.

joachim → created an issue.

Looks good. Thanks!

joachim → created an issue.

Just a couple of minor things to change.

I don't understand the question, sorry.

It's been a while since I posted this, so I don't remember it much, but from my original post, the problem is this:

channels:
  node:
    - my_bundle <- %key of this is '0'.

channels:
  node:
    - my_bundle <- %parent.%key of this is 'channels'. Why is it not 'node'?

> public static function getCode(PackageInterface $root_package, InstalledRepositoryInterface $repository): string {

We'll need to add a parameter to that when 🐛 Wrong DRUPAL_ROOT with non-standard code structure Needs review joins in to add its own data to the DrupalInstalled class, probably the ManageOptions object. But that's fine to happen in that other issue, as the class with this method is marked as @internal.

> scaffold does not run if you install Drupal from core git repo

How do we resolve this?

Wrong project.

Note that if you're just using a flag as a UI to trigger the ECA action and you don't care about the flagging entity that doing that creates, you'd be better off using https://www.drupal.org/project/action_link → instead.

This is absolutely still relevant. There is no proper documentation on plugin types the way that there is for hooks.

Nobody has worked on it because it's a big undertaking.

    return t("@controller controlling @dependents", [
      '@controller' => $controller_filter,
      '@controller' => $controller_filter ?: '',

There's a follow-on here:

If '@controller' is an empty string, then the UI text makes no sense.

Fixed.

The IS should explain the consequences of this bug more clearly. For instance, top-level constraints are not validated with this method:

$violations = ConfigEntityAdapter::createFromEntity($config_entity)->validate();

Also, the CR needs some editing for style. It should explain what is new, and how it has changed. The chatty style isn't appropriate.

It's been a long time since I wrote this module, so I don't remember how it all works.

But even entity types that have no bundles actually have a bundle -- it has the same name as the entity type ID. I think the change should be doing something with that.

This doesn't cover config entity validation which was added recently.

Have you looked at the README file in the module?

Also, there's no need to upload screenshots of a UI the maintainers already know about!

joachim → created an issue.

joachim → created an issue.

I think a plugin type for extracting files from a node is definitely the right way to go, as we have several issues where we need to expand how files are obtained:

- layout builder items - ✨ Make it possible to index attachments from index only fields that don't exist on the entity Active
- nested references such as paragraphs: ✨ Impossible to index attachment in nested entity reference Needs review

There's a couple of important changes I'd say the MR here needs though:

1. It should be possible to have multiple plugins active. For example, you might want plugins from layout builder AND from files referenced in body text tags.
2. Some plugins need to be recursive, and run enabled plugins on what they find. For example, you might want the paragraphs plugin to then run the body text plugin on the body fields on the referenced paragraph entities.

Sorry, it was 2 years ago at a firm I'm no longer with.

My best guess is that I had some array processing or extracting process plugin twice in the same pipeline.

Could the MR branch be rebased rather than have 11.x merged please? It keeps the history easier to read.

> Now you got a number characters left. Half is for the first part of the table/alias name and the rest is for the last part of the table/alias name.

That might cut off words in the middle, which is going to make some ugly table names.

I think it's better if callers can specify which bits of the table name they want to preserve -- it's fine if that's bits at the start AND at the end.

> To make the change as useful as possible for the views module, the last part of the original name/alias is very important. For others it is the first part. Therefore my suggestion is to replace the 'middle' part

That's fair enough. But what's the algorithm for deciding what counts as the 'middle'?

@daffie your suggested change is a big change to how this was envisaged to work:

   * When a table name or alias combined with the database prefix is greater
   * than the maximum from the class variable $maxTableNameLength, the name
   * will be abbreviated. The abbreviation will remove the middle part of the
   * name and replace it with a 10 character hash. The first and last parts of
   * the name will be preserved.

What counts as the 'middle' part? What if the first and last part don't guarantee uniqueness? I think it's best to leave it up to callers to know which part of their table name must be kept for uniqueness.

Another place in core where we've for a concept of plugin type: \Drupal\views\Views::$plugins defines the plugin types that Views invents, and has metadata about those types.

The code in this MR should be moved into a plugin once ✨ Plugin type to allow other ways to determine files to be indexed Active is in.

This MR's approach of using a computed field to provide the files to SearchAPI is interesting, but I don't think it's the right way.

We've got multiple issues that are all to do with 'how to get files out of the entity':

- this one, about paragraphs
- layout builder: ✨ Make it possible to index attachments from index only fields that don't exist on the entity Active
- files linked in body text: ✨ Plugin type to allow other ways to determine files to be indexed Active

I think the approach needed is to have FilesProcessor hand over to a set of plugins which handle 'getting a PDF file from the current entity'. There's an MR for that over at ✨ Plugin type to allow other ways to determine files to be indexed Active

Fixed link

I'm not sure, as this one is about subprocess specifically.

joachim → created an issue.

The failing test Drupal\Tests\file\Functional\DownloadTest is passing for me locally with this MR, and I don't see how it can be related anyway. Must be some glitch in the testing system.

> // If $form_state->isRebuilding() returns TRUE and input has been processed

This is correct and accurate, but from the POV of DX, it doesn't fix the problem.

The original docs approach this from answering the question, 'what do you need to do to the form to get this behaviour?'

> If $form_state->isRebuilding() has been set

It's just that the technical detail is wrong.

We need to change the detail, not the meaning being conveyed.

Thanks!

But all the @return docs must be a single paragraph -- see https://www.drupal.org/docs/develop/standards/php/api-documentation-and-... → .

> We shall need to be very careful that we do not break any core/contrib/custom code.

The MR isn't changing any existing behaviour:

- call createTable() -- same behaviour as before. If the table name you supply is too long, then Bad Things happen!
- createAbbreviatedTable() -- new API, ensures that your table name is not too long.

For follow-ons, the rule is:

- were you passing table names to createTable() that MIGHT sometimes be too long? When the table name was too long, then your code was crashing anyway. Change to calling createAbbreviatedTable() to prevent crashing
- were you already passing hashed table names to createTable()? If so, you need to either:
- a. Keep doing that, no change
- b. Change to calling createAbbreviatedTable(), and rename your tables in an update function. Proceed with care! This can only really be done if your table names are internal. If contrib or custom code might be referring to your tables, there's a problem! We need a way to deprecate table names!

The problem was that it needed a rebase -- 11.x had a new feature for week granularity, and the test for must have got pulled in by merging 11.x into the branch and not properly resolving the merge conflict.

I've rebased and resolved the merge conflict and the test passes now.

Please post text rather than screenshots! I can't copy a class name from a screenshot!

The wording is good, but @note is not a docs tag supported in the list at https://www.drupal.org/docs/develop/standards/php/api-documentation-and-... →

Oh I've just realised you're talking about migrating to version 3. I got the version numbers mixed up -- I was thinking of version 4.

The entity field storage system currently hashes its own table names, right?

In which case, can't it just carry on doing that if it wants to? Using this new API is optional.

A follow-up could take care of renaming those tables and switching the entity field storage system to using the API.

I tried a very simple script to get an idea of memory cost:

$autoloader = require_once 'autoload.php';

dump(memory_get_usage());

\Composer\InstalledVersions::isInstalled('drupal/core'); // returns bool

dump(memory_get_usage());

The result is:

 2545632

 3568056

so that's 1MB of memory for loading the Composer runtime. It's not huge, but it's more than 1% of our minimum memory requirement.

Also, 📌 Use a better container cache key Active is looking at writing a file with constant data at Composer install time too, so I think it makes sense for the two issues to converge.

Looks good. Thanks!

You can prevent a mail being sent with https://api.drupal.org/api/drupal/core%21core.api.php/function/hook_mail...

For each PHP snippet, you need to create a Computed Field plugin. You can use Module Builder to generate the scaffold for the plugin classes, and then you'll need to copy the PHP manually and tweak it a little for the incoming variables.

> 1. Modify createTable() to check the table name length and, if necessary, prompt users to provide a prefix before automatically calling createAbbreviatedTable().

This is an API. We can't prompt users. And in any case, users shouldn't be exposed to internal details like database table names.

MR looks good. Test failures are unrelated JS issues.

joachim → created an issue.

joachim → created an issue.

joachim → created an issue.

joachim → created an issue.

So the problem is coming from:

1. SearchApiDisplayDeriver doesn't get a Search API display plugin for the Views display, and so it doesn't derive a facet source
2. There isn't a Search API display because search_api only derives Search API display plugins for Views displays with core display plugins, i.e. block, page, feed, etc. views_block_override module isn't providing the integration.

So technically the problem is with views_block_override, but Facets module needs to handle this scenario without crashing.

joachim → created an issue.

Looks good!

Thanks.

I've thought about it some more, and here's what I think we should do:

- add the channel entity token so we don't need the core patch
- implement the hook like in your MR, but have it hand over to the finder plugin. That way:
-- particular finder plugins can override it if necessary
-- if someone really wants it to be optional, they can either hack it out in a custom plugin, or add a setting for it in the finder config entity

> Letting localgov_events set those patterns and freeing finders from the responsibility of doing anything with URL aliases could be an option.

I'm not sure about whether Finders should take care of this. I think I said it github that it should, but I'm wondering whether that's too opinionated for Finders.

We could work around the core patch problem and also improve DX (and UX if we go down the route of users setting it up themselves) by providing our own token for 'node:finders_channels:0:entity'.

Aside:

    $bundle_id = $entity->bundle();
    $bundle_entity_type_id = $entity->getEntityType()->getBundleEntityType();
    $bundle_entity = $this->entityTypeManager->getStorage($bundle_entity_type_id)->load($bundle_id);

    /** @var ?\Drupal\finders\Entity\FinderInterface $finder */
    $finder = $this->entityTypeManager->getStorage('finder')->getFinderForBundleEntity($bundle_entity);

Maybe we need a helper to do all of that? Not sure where I'd put it though.

> The thing is, with finders, the content types set up as entries have to be in a channel, so we don't need to do this exactly like directories.

Is the channel field optional in Directories? Should we have done that in Finders too?

Thanks for the report and the MR. For a crash, status can be 'critical' :)

Core changed this in 🐛 BreadcrumbBuilder::applies() mismatch with cacheability metadata Active -- it's a pain as it's impacted other contrib module as well as Entity UI.

Committed, and will make a new release now.

(BTW remember to set an issue to 'needs review'.)

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

This has broken several contrib modules that inherit from PathBasedBreadcrumbBuilder --

- 🐛 BusinessRulesBreadcrumb must be compatible with PathBasedBreadcrumbBuilder on Drupal 10.4.0 and above Active
- 🐛 Fatal error: Declaration of Drupal\entity_ui\Breadcrumb\AdminBreadcrumbBuilder::applies Active

Linked to the other page -- this is a total mess though, needs more clean up.

Also, remove the jovial tone and use of 'easy' -- not appropriate.

mention development.services.yml

joachim → created an issue.

+1

-1 from me.

The improvement to readability happens every time a developer looks at the code in question. That's a frequent benefit.

The downside of a merge conflict happens only when that code is changed. That's a rare cost.

Done.

That was possibly done for the typed class constants I'd put in, which I later removed.

Try it on PHP 8.1 and see what happens :)

Though honestly, PHP 8.1 is security releases only, and will be EOL in 10 months' time.

joachim → created an issue.

joachim → created an issue.

What about the actor not being an entity at all?

My use case is a personal blog site. I want to configure which node types are posts, but I don't need it to be tied to a user account.

There is https://www.drupal.org/project/site_entity → which provides a singleton site entity, but it doesn't have any releases and looks abandoned.

joachim → created an issue.

joachim → created an issue.

Uploading a patch on 3.8.x so it can be used on D10.