🇳🇱Netherlands @ekes

ekes → created an issue.

If it helps I just landed on this issue because I got a load of :third_party_settings.field_group.group_category.format_settings.effect missing schema errors with 4.0.0

So if core isn’t causing this wouldn’t it be a bug in the contrib or custom module that is causing it?

As they are following the Schema API documentation. I don't think so. It would require a change to the requirements as documented for API (as quoted above) where it states that you can configure it with no fallback key set.

Change like this usually means an isset() is masking a larger issue shouldn't we address that so it's always set.

It's not going to be core code that is calling it with the key not set, because the reason for it not to be set is if you have a column that is only for a specific database backend, and has no fallback. The documented configuration for columns with no fallback is not to set one. So I'm not quite sure how you can make it always set? Changing the documentation (which I guess is an API??) to require a NULL when there isn't one at a guess?

ekes → created an issue.

Hmm... ignore me it must be some strange composer dependencies, I already did it! 9c7e22a9bae054226a465f6d578589cd4e31d713

ekes → created an issue.

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

I've just triggered this warning as well:

1) /var/www/html/web/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php:2567
Undefined array key "type"                                                                                                                                              

2) /var/www/html/web/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php:2570        
Undefined array key "type" 

but am confused by the OP.
@lendod Did the patch on the Role Delegation module make any difference to your error?

The code that is triggering this error is:

  public static function castValue(array $info, $value) {
    // Preserve legal NULL values.
    if (isset($value) || !empty($info['not null'])) {
      if ($info['type'] === 'int' || $info['type'] === 'serial') {
        return (int) $value;
      }
      elseif ($info['type'] === 'float') {
        return (float) $value;
      }
      return (string) $value;
    }

    return $value;
  }

The $info['type'] in question here is the schema definition of the field as described in the Schema API documentation. Specifically:

'type': The generic datatype: 'char', 'varchar', 'text', 'blob', 'int', 'float', 'numeric', or 'serial'. Most types just map to the according database engine specific data types. Use 'serial' for auto incrementing fields. This will expand to 'INT auto_increment' on MySQL. A special 'varchar_ascii' type is also available for limiting machine name field to US ASCII characters.

Now according to the same documentation you can leave this unset. Which is what was happening in my case (the column has a database type check for 'mysql', so will only install on a MySQL DB, and includes a mysql_type key on the column field definition).

'mysql_type', 'pgsql_type', 'sqlite_type', etc.: If you need to use a record type not included in the officially supported list of types above, you can specify a type for each database backend. In this case, you can leave out the type parameter, but be advised that your schema will fail to load on backends that do not have a type specified. A possible solution can be to use the "text" type as a fallback.

So in the OP I wonder if it wasn't some other field that was causing the issue. I don't see anything obvious in Role Delegation that is adding more than a computed field.

From the above I'd propose to make the MySQL backend fit present behavior (but without a warning) and also fit the documentation a isset should be added to the above code, making it.

  public static function castValue(array $info, $value) {
    // Preserve legal NULL values.
    if (isset($value) || !empty($info['not null'])) {
      if (isset($info['type'])) {
        if ($info['type'] === 'int' || $info['type'] === 'serial') {
          return (int) $value;
        }
        elseif ($info['type'] === 'float') {
          return (float) $value;
        }
      }
      return (string) $value;
    }

    return $value;
  }

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

ekes → created an issue.

Follow-up issue applied.

Tests need checking. Sounds like you've been using it @myha. So I'll close this, and make a follow-up ticket to checks before tagging a D11 release

OK this is easy to review. I'd made the same mistake somewhere else, and did the same fix. LGTM.

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

Patch at this point

ekes → created an issue.

My only further thoughts are if we need per bundle permissions. As we now have quite a few Geo bundles (mostly for connecting and importing the data from ArcGis). So generally we would not want normal Drupal editors to have permission to delete those. Happy for that to be an advanced permission sub module.

I'd that sounds reasonable, so making another issue / PR would make sense. Another suggestion (not for create so much as edit) was if the user had access to an entity referencing the geo entity... although as we don't have back references I suspect that might be too complicated.

!14 is easier to read :-) Still would be good to get that JS cleanup though.
Administer permission I'd overlooked the early return. Makes sense.

Just logging review messages:

• Lots of nice code tidy up, the JS for sure needs it, but makes it harder to read the substantive changes. The YML for sure has inconsistent ' ".
• Upgrade path should probably be in a post_update hook as it's working on loaded entities.
• Does it also drop the 'administer geo' permission meaning that a user can update, delete (but leave it for create)?

I'm guessing the intention here is to redirect to the admin list of messages? The old code linked to by @ayalon did use the view list route view.message.page_1. But of course that can be deleted/replaced etc. which would lead to this same issue.

Options I can see here are:

Maybe if there was a link template for collection defined, with some simple list, that can then overriden by the view, but there will always be the route. Then it could go to the collection list?

Or easier redirect to the front page or admin page by default. And use the ?destination= on links from views etc. to redirect back.

Not sure what the plans were? But switching either way would fix Drupal\message_ui\Form\MessageDeleteForm and Drupal\message_ui\Form\DeleteMultiple that both reference the non-existing message.messages

finn lewis → credited ekes → .

borisson_ → credited ekes → .

Looks good. The failing tests are because of upgraded PHPUnit, so they need fixing separately.

I still don't understand how this differs from the form that Group automatically makes like at for example `/group/1/content/create/group_node_request%3Aarticle` and `group/1/content/add/group_node_request%3Aarticle`. But if (re)using these forms doesn't do what you need. If you could open a new feature request issue, and describe what is needed that is different from those forms I think that would help.

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

I've removed that form. If it has value we should remake it so that it asks which group to add the entity to - I think?

I found more grequest overlaps so fixed them at the same time, and added a couple of tests.

I'm not 100% that they've all be cleaned up. So I'll roll out another alpha.

If you spot some more do open another issue, MR.

It helps, me at least, if you keep the MR with multiple individual commits, for each issue/problem/thing-being-fixed, rather than as one bigger commit.

The autoloader not being updated is, from my understanding causing quite some confusion when geocoder plugins, which are not modules, and are installed only with composer, are sometimes not detected. 🐛 How to add Gecoder 3.x providers Needs review

The issue I described in comment #33 https://www.drupal.org/project/geocoder/issues/3153678#comment-14203727 🐛 How to add Gecoder 3.x providers Needs review where you install the plugin using composer, but the autoloader is not rebuilt, so it's not detected, should I think be fixed with:

📌 Use a better container cache key Active

I'll close this in preference to above.

Potential solution here: 📌 Use a better container cache key Active

I'm wondering about the form group-request-node. I notice I'd made a comment in the test that the module should probably use the default group generated forms.

Did you just have the error when the module presently called this form.
Or is there a good use for using a different form?

Hi,

Thanks for this, it's quite likely there are issues with grequest (as lots of the code came from there). I'll try and get to test it soon. One thing I noticed immediately though. Could you not use {$plugin}->getBaseId() in place of getPluginId() and then looking for the str_starts_with?

> The issue is that the entity browsers ignore this setting, as they are just a view. We have found (due to our use of many geo entity types)
> that we have had to create separate entity browsers to avoid this.
> Wonder if its possible to send something from the field configuration to the entity browser view to apply a filter.
https://github.com/localgovdrupal/localgov_events/issues/186

Removing the canonical link template, and changing the line in module_builder_entity_type_build() has also so far worked.

FWIW I just had a quick look at media entity (which also sometimes has canonical the same path as edit-form) it does still have a view builder defined in the handlers... ah that's for the case where it does have a standalone url. In the other cases the route_provider class returns the edit form route for the canonical route: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/media...

Which module is that? Could be. It's a council's website.

I just installed Module Builder, to generate, a new module and a couple of plugins, but this time I'm not getting past submitting the new module from. Removing the link template, the form submits, but then just leads me to a whole other set of errors however.

I'm getting this on a new module at the moment.

I note there is a canonical link template:
https://git.drupalcode.org/project/module_builder/-/blob/4.0.x/src/Entit...

But there isn't a view_builder handler:
https://git.drupalcode.org/project/module_builder/-/blob/4.0.x/src/Entit...

Both of which are required to build the route in the inherited route_provider.

Should be fixed now.

Not quite. Now it's kicked the Drupal Tests back into action phpunit is failing.

Thanks for the review. Looks good. LGTM.

finn lewis → credited ekes → .

markconroy → credited ekes → .

finn lewis → credited ekes → .

finn lewis → credited ekes → .

ekes → created an issue. See original summary → .

This is still valid after 🐛 User 1 isn't the super user anymore in Drupal >= 10.3 Active . Although it needs to use $this->getAdministator() to get the correct uid to insert.

The issue was, and still is, that if the UUID of the required owner field of an exported entity is not in the importing site the the value of the owner field is discarded, the required field then has NULL - hence failing to import.

I use "owner field" here. It's usually uid but I'm not sure it has to be does it? It is however maybe the most common case of a required field referencing a uuid that might not exist in the system that can reasonably be reassigned to another (default admin user) reference.

finn lewis → credited ekes → .

finn lewis → credited ekes → .

> I took that using constructor property promotion should be encouraged for new code

+1 for that interpretation. We did already make that assumption in LGD code; plus imho it's soooooo much nicer.

Related 🐛 Date augment, remove occurrence Active

So in my digging into how these things can be done. I've logged what happens with the ical feeds of a Google calendar. Overriding, changing, and removing. Figure I'll log it here too as it might be useful.

https://gitlab.com/ekes/2025-date/-/blob/notes/ical-rrule-override-seque...

Referencing the RECURRENCE-ID works here, as it references the time in the sequence it would have happened, no matter other individual alterations, including removal. Google does not try and reconcile overrides if you change the underlying sequence. Although interestingly it seems they might remain orphaned with no data, as if the underlying sequence returns to the same start times, the empty overrides reappear in the feed.

finn lewis → credited ekes → .

ekes → created an issue.

ekes → created an issue.

Reviewed and spotted one last nullable type. For the rest looks good.

Note this does not change the function signature see my note about this on https://github.com/commerceguys/addressing/pull/221#issuecomment-2582855457 so shouldn't require more than a patch release.

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

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

Will require https://www.drupal.org/project/date_recur/issues/3498936 🐛 DateTime Range Views Data was converted to a class, hook removed Active

Passes PHPUnit.

I don't thing the cspell, phpstan, phpcs are related to this change?

ekes → created an issue.

ekes → created an issue.

ekes → created an issue.

ekes → created an issue.

Was just coming here to do something just like this. Patch adds nullable to all declarations that have a NULL default, and shuts up our 8.4 test errors \o/

I'm just trying to recreate this. Are there some steps I could follow?

Shall open a new issue for any remaining/new.

ekes → created an issue.

\Drupal\Core\Render\Element\FormElement::* is deprecated in drupal:10.3.0 and is removed from drupal:12.0.0. Use \Drupal\Core\Render\Element\FormElementBase::* instead. is address module.

The "Drupal\entity_browser\Entity\EntityBrowser::*" method will require a new "string *" argument in the next major version of its interface "Drupal\entity_browser\EntityBrowserInterface", not defining it is deprecated. is Entity Browser module.