Switzerland
Account created on 9 December 2007, about 16 years ago
#

Merge Requests

More

Recent comments

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

You can't inject into an entity class.

This requires 10.1, so the .info.yml will need to be adjusted accordingly, will most likely only commit this closer to the D11 release.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

The handling of a possible error is likely wrong in \Drupal\pathauto\Form\PathautoBulkUpdateForm::batchFinished(), multiple issues with it, that print_r() won't do anything useful and the error operation needs to be converted to a string too.

I'm not entirely sure how to even produce an error in a way that ends up there, but if you do, debug it and figure out how to display it.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

There is no new option, all entity types with a canonical link template should show up there and redirects shouldn't have that unless you customized it somehow.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

I've been pondering whether using key value was the right decision, but the performance impact of this shouldn't be that big.

key_value queries should be well optimized and pathauto state should be acessed only when editing/generating aliases.

Also, have a look at πŸ“Œ Use cache collector for state Needs review , I've been working on that for years and I'm hopeful that it will finally land soon.

This is a fairly big change with a possibly slow migration path, so it would need to be really worth doing so.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

Caching won't help, we don't want this to be outdated and it still needs to be cached.

What I'm open to implement is disable certain features of the report page if there are more than 1M keys for example, like the per-bin counts. Cache tags are already capped. Or we could add a cap to the bin as well, with a warning, would still give you a rough idea about the distribution.

Also, the report could also be implemented as a drush command which doesn't need to worry about timeouts so much.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

@longwave: Plugins aren't (single-instance) services. Every node you load is a plugin object, so are fields and blocks and so on.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

I think the UI question is entirely disconnected from this issue. It's only related because it requires special workarounds to not trigger the deprecation because config entity forms by default just throw everything the form defines directly onto the entity. We could in theory revert this change in D11, it would just set an undefined property on the object but won't do anything with it (like many, many other form elements)

I do agree that this is special, but who can access which text format is IMHO such a central question that I definitely see this being useful here. That said, we have a pattern now with a permissions local task on e.g. node types that we could also use for this, it would be a bit overkill as it's always going to be a single permission only though.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

#20: Yes, both a dynamic internal and dynamic parent list of properties doesn't work for EntityType. That said, I'm not sure if I even implement that yet, I think it would also be acceptable to require that extra stuff is manually placed inside additional when defined on as an Attribute.

If we want to won't fix this then fair, it just means that we can't introduce generic new features to the plugin system the way πŸ“Œ Create a way to declare a plugin as deprecated Needs work currently does, each plugin type that wants it will need to define it. That might be OK, because it will only actually do anything if there is logic for it, for example what we had in the block UI, where it didn't show deprecated plugins unless they were preconfigured.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

A specific plugin system can always define additional keys on top of this, like a replaced_by or something. For most other plugins that's likely not needed or more complex, as plugins usually have configuration and the configuration of the replacement might not apply 1:1 and so on. So I'm not sure if that we need to build this into the base functionality.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

@alexpott as long as most of those are not reusable that's ok and the block deriver won't change much, only a lot of reusable blocks is an issue.

Meaning, an index isn't a bandaid, the other issue would just move the query to the block config autocomplete and block content list has it too.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

> And what about the upgrade path? Don't we need to change the configuration of the existing installations?

No, because as stored configuration, the property isn't there anyway, This only affects default config files, and you can't files in an update function.

> I've kept it in \Drupal\Tests\ckeditor5\Kernel\SmartDefaultSettingsTest since it loads config from a folder called ckeditor4_config and don't want to mess up anything there. If it's ok to remove it, let me know.

I think it's fine to change that. the roles/permission are not actually tested by that test I think, they look like they were copy pasted from standard profile (an old, ckeditor4 version of them). You most likely don't even need to add any permissions, unlike some of the other tests I commented on.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

That's a known issue with overridden base field definitions and updating, but I think when updating from Drupal 8. πŸ› TypeError: call_user_func(): Argument #1 ($callback) must be a valid callback Needs review is a similar issue, additonally also with a class name change there.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

Note that using the gitlab UI to merge does by default not use the commit message format that's common in drupal, while having authors mentioned there or not is being discussed, but having that and the issue number allows to generate release notes with https://drupal-mrn.dev/

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

Pushed an alternative suggestion as a new MR.

We already have the entity object and its definitions, we can loop over them, just like getFieldNames(), there is no need to query/load field definitions separately.

I also did include the static cache change for field names, but not the persistent cache, I'm pretty certain that a cache get is more expensive than a loop over field definition objects. I'm not even certain that the static method is worth it, quite possibly not.

Locally in profiling, this method and its whole call chain no longer appears in blackfire which this change, but it's also worth pointing out that it was _way_ faster locally than the blackfire reports from production in the first place.

I also changed the method to a generator but that could easily be changed back if that's deemed too much of a change.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

Just stumbled over this as well in profiling, makes up about 10% of the time on the one example page I looked at.

The cache as it is does not use cache tags, so it doesn't account for field changes. One option is the use the entity_field_info cache tag to handle that.

That said, what my plan was before I found this issue is to use the field storage definitions from the entity field manager and then check the type and target type setting in PHP, I think that should have minimal overhead as those definitions are very likely then already loaded at that point and a loop over that should be pretty fast.

I'm uncertain if the extra cache on getWebforms() is worth it, do we have any data on that? _Maybe_ if the fields aren't initialized yet, but cache request aren't free.

The constructor BC fallback is also not using the same bin as the service definition.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

This seems to only add support for annotations and it relies on undocumented, undefined annotation properties to do so.

We need to make sure this works on attributes as well, which in its current state, I believe it does not, because for example the just committed Mail attribute class in πŸ“Œ Convert Mail plugin discovery to attributes Fixed has a fixed, hardcoded set of properties defined in its constructor and it is not possible to just pass in additional stuff the way this expects it to.

I think we need to do either or both of:

a) Standardize on using ...$var to pass all extra constructor arguments through to the parent. See for example ConfigEntityBase in πŸ“Œ Convert entity type discovery to PHP attributes Postponed , so that the attribute base class can introduce and define the deprecated_message property and it works an all child classes too.
b) Support an explicit additional key, which is considered here in the trait in the parent class, so that extra stuff can be put there. We discussed that a bit in the original issue but postponed its introduction on having use cases for it, more or less defining that it would be done for specific plugin types that are likely to be extended.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

drive-by-comment, maybe this should be combined with any of the specific plugin classses, possibly one with only few plugin implementations? Just the base class wont' really give us anything to test it with.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

> .asp / .aspx (especially because .aspx exists)

.asp exists too, that's why requests to it are made. All of the default patterns are things that exist somewhere. Maybe there should be a bigger emphasis on reviewing and adjusting that configuration in the documentation/project page. If you recently migrated from a wordpress site to drupal you will likely get valid wordpress related request as well and might end up blocking users and crawlers if you're not careful.

Yes, regex is a problem, exposing regex directly basically makes this a developer tool. But regular expressions are also flexible in ways that the default drupal path matching isn't, I'm not sure if that could cover the current rules 1:1 or those that people have set up. One option would be to have two different settings, one for regular expressions, one for standard drupal path matching. I don't think that belongs in this issue though. Another idea would be to have a test UI, where you can enter a path and it lets you know if and which rule matches that path. but belongs even less in this issue.

FWIW, directly exposing regular expressions in the UI can also be a security issue, but it's already protected with a restricted-access permission.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

Berdir β†’ created an issue.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland
πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

Berdir β†’ created an issue.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

Added that. Wasn't sure if it should use the same flood key or not, but then it should probably just be perimeter.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

That's how regex works? If you want that to not match aspx, then you should end with a $, which means end of line, so only matches if the string ends with that. Same for ^ to match on start of the path.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

Install went fine on our install profile (138 installed modules and themes), didn't run tests yet.

before:
 [notice] Performed install task: install_profile_themes [35.95 sec, 91.52 MB]
....
 [notice] Performed install task: install_finished [62.81 sec, 178.25 MB]

after:
 [notice] Performed install task: install_profile_themes [24.95 sec, 99.71 MB]
...
 [notice] Performed install task: install_finished [50.77 sec, 166.01 MB]

Total install time wennt from 62s to 50s, but as you can see, around 50% of that time is installing default content and stuff like that. module install itself went from 35s to 24s.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

Did a reroll as a merge request for 10.2.3+, still test fails to figure out.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

left some comments.

Personally I would definitely recommend to not do that much in a single MR, it's impossible to review. I only scrolled through a bit.

My preferred approach is to just enable tests with required test fixes and everything else in follow-ups, it's just warnings. But it's a bit late for that now, your call.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

Yes, this is working well for us. I now have a transport plugin with my own unique DNS prefix/name, and a corresponding transport plugin that instantiates an Smtp factory and then adds the authentication thing.

Name of the tag is your call I guess, I still think that the problem outlined in #13 which would result in multiple extra factories being registered could be confusing or even break something once the core issue is committed. Up to you.

BC is a good argument per #15 to keep the current call, the implementation as it is now still allows to entirely replace a given default factory with a service. And the whole thing is temporary, once core provides the factories then I guess this module can do a major new release, require 10.3 or whatever and build on top of that.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

Here is a quick hack to override that position *again* within ckeditor, but I'd rather find a way to not override the position three times, but that needs someone more skilled than me with CSS. It looks like the autocomplete element there doesn't have a useful wrapper that we can target.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

It looks like that was added in πŸ› Linkit field widget expands to 100% of page width Fixed , but maybe it needs to be specific to link elements then?

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

We did have this as well sometimes and sometimes not on a given site.

The background is not really the problem here. The main problem is the position definition. The

    with the suggestions needs to be position: relative so that it's within the container so that it expands and then also provides the white background.

    What we found while comparing is on pages where it works vs. where it doesn't work is order of CSS definitions that are being applied. There are multiple overrides in place here, linkit has these two definitions:

/* Override the default jQuery UI theme */
.linkit-ui-autocomplete {
  max-height: calc((100vh - 80px)/2);
  overflow: auto;
  position: relative;
}

.linkit-ui-autocomplete.ui-widget {
  font-size: .9em;
  max-width: inherit;
  position: absolute;
}

So it's set to relative, and then with ui-widget immediately changed to absolute again. I don't know why.

The whole thing is then again combined with the reset CSS:

.ck-reset_all :not(.ck-reset_all-excluded *), .ck.ck-reset, .ck.ck-reset_all {
  box-sizing: border-box;
  height: auto;
  position: static;
  width: auto;
}
.linkit-ui-autocomplete.ui-widget {
  font-size: .9em;
  max-width: inherit;
  position: absolute;
}

These two definitions seem to have equal weight/specificity, so it depends in which order they are applied. if ck-reset_all is first, then it works, if position:absolute wins, it does not.

As #9 says, there is something weird with the reset class that's not actually working, but not having that wouldn't really fix it I think.

Any reason why we can't just remove that position:absolute?

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

> In a contrib module

No, in a *contrib* module you don't want to use bundle classes for user or any other entity type that's not your own IMHO, exactly because these are not decorators. You can only have exactly one bundle class active for a given bundle. So if you have two contrib modules doing that, one of them would win and the other would not be used, breaking code that depends on it.

Bundle classes are for custom modules and I guess possibly your own entity types/bundles in contrib modules if you have predefined, specific bundles.

If we want to allow contrib modules to add some methods to other entity type classes then we would indeed need such a decorator system, but that IMHO comes at a price of complexity, decorators need to be correctly implemented and add extra method calls for every single decorator that is there, as each method call is passed through all decorated classes, which also makes things even harder to debug around entities.

A better approach for that might be an Adapter pattern IMHO, so if you have subscriber related logic for a user, you can do new SubscriberUserAdapter($user), and then use your methods and just those on a user when you need them. It's an extra line in your code and then only exposes the specific methods you defined, but it's also not overhead to pass through every single time anyone does something with a user.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

FWIW, that said, we are using this patch now in production and did not yet get any complaints about it, so it's definitely an improvement over HEAD and one option would be to commit this and reopen πŸ› Drupal Commerce purchase event gets sent multiple times if someone refreshes the checkout success page Active to implement a persistent deduplication using tempstore or something.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

@japerry: I'm not entirely sure that the purchase and this event are exactly the same and need the same fix per #4. Purchase definitely should not fire more than once ever for the same order, but I can see use cases where this might be desired for this event, like when you abort and re-enter checkout.

But if you have a payment and then return on a failed payment to the first step then that's also a problem and likely a bigger one than not firing the event again.

commerce_google_tag_manager did not have a persistent key value store, it just used a request subscriber and only acted on GET requests, which is not unlike the current patch here.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

> From https://git.drupalcode.org/project/drupal/-/merge_requests/5636/pipeline... looks like we're saving approx 10 database queries on nearly every page request, which is great and what I'd expect based on how many state checks we do usually.

Yes, saw that too. I was confused about the increase of queries in that umami test though, I need to look at what that exactly does, I wonder if there's some timing issues/state cache race conditions as it does multiple requests in quick succession.

Reproducing that manually in blackfire with a user without any permissions in umami gives me _very_ different numbers and what I expect. -4 state queries, +1 cache_default query, 33/3 in total. And not 17/18.

I think now that it's opt in, it would make more sense to move this to cache_discovery so we use the fast-chained backend for it? That should save another query then.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

It's a bit ugly, but I implemented the setting now in State, have to override a bunch of methods to avoid unnecessary cache get/set/invalidate calls.

This should have an effect on the new performance tests in core, haven't looked much at them yet, I guess that's going to change now.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

Berdir β†’ made their first commit to this issue’s fork.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

Reroll.

The sleep issue is moved to πŸ› Hardcoded sleep on webhook callback blocks processes Needs review . The patch there conflicts with this though.

This is working quite well for us, but note that it changes PaymentSdkInterface, so technically has BC breaking changes.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

PHP 8.3 is supported by Drupal 10.2 and people are reporting bugs with it for contrib modules. I think it makes sense that PHP_MAX is what it says in the name, the MAX/most recent supported PHP version for a given core version?

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

Merged.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

Berdir β†’ made their first commit to this issue’s fork.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

Will need a rebase after a recent fix to update_cron definition.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

No idea that's due to a mysql sql server or something, but as #4 said, you can't just convert that to a float if it's not.

apparently hasn't happened again in 7 years, so just closing that, feel free to reopen if it's still happening and can be properly fixed.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

I can't fix problems that I can't reproduce. non-defined or incomplete entity type definitions usually happen when there are other issues with a site that prevent full discovery or break alter hooks.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

Still unable to reproduce this. The module just implements hook_schema() like many other modules.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

Why not, although wondering if it wouldn't make more sense to pass the log entry along to the hook instead. But committed.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

Doesn't apply anymore.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

Merged, but no idea how accurate docs docs still here, they haven't been updated for D8+.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

Why not I suppose ;)

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

Correct me if I'm wrong, but I think drupal console is pretty unmaintained now and everyone switched back to drush? Therefore not going to add that to the project as something that would need be maintained and updated.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

Berdir β†’ made their first commit to this issue’s fork.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

Berdir β†’ made their first commit to this issue’s fork.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

I'm not certain this is a good idea. This changes config, it's not just cached data, and could be unexpected. And it does exactly the same as the button in the UI.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

Interesting, needs tests updates and some screenshot with a mix if various settings would be useful, to see a bit how that looks and how big it gets.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

We don't schedule cron, we schedule jobs when cron is run, which typically happens from an external system and we don't know when that is. We could guess automated cron rules, but that's not reliable and as mentioned elsewhere, if you have complex enough cron scheduling requirements that requires ultimate_cron, then you really shouldn't rely on that.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

By default those e-mails are sent immediately, but if you do anything it push it to cron/queue then they will be sent as frequently as you call cron.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

Yes, likely a duplicate of that.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

Committed.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

I don't think it is necessary to uses a separate channel for every job, that could be part of the message.

watchdog messages are translatable, they must not contain dynamic parts or every unique message will be its own separate translatable string. This could also be a security issue.

Instead, formatInitMessage() needs to be re-implemented to use placeholders.

Not convinced that this should be built into the Database logger. Either it should be it's own logger plugin (but then you can't have both I guess) or a separate setting.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

That post is pretty good I think, I linked it from the project page now. Maybe it could be moved to a drupal.org documentation page, so it doesn't get lost.

The blocker is in, setting to needs work for the missing parts.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

Berdir β†’ made their first commit to this issue’s fork.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

Closing as duplicate of πŸ› Invoke hooks via the 'module_handler' service Needs work which might work for hook_event_dispatcher as well and the long-term plan which is πŸ“Œ Support CronSubscribers Active

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

Related issues: ✨ hook_event_dispatcher compatability Active and πŸ“Œ Support CronSubscribers Active (and the related core issue).

Interesting solution but it's trying to be a little bit too generic I think. Whether or not something is/has certain hook is just a naming convention, with that logic, any module-prefixed callback is going to be recognized as a hook.

Instead, we should do this only if it the cron hook. Meaning, isCallbackHook() should just check $this->getHookName() === 'cron', at this point we can of need to expect that it exists.

Hopefully, the long-term solution is the core issue that I created to make cron tasks/jobs their own thing that can be discovered and not hooks. Feedback/reviews there would definitely be welcome.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

Makes sense.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

This breaks the updated tests which now use such a queue plugin in the edit test. This also means that we should able to test this pretty easily.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

Committed this, prefer the changed separator over keeping __ of the other issue.

This does mean that existing jobs will be duplicated, but they weren't working before, so that's fine by me.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

Merged.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

You are not meant to be using the simple scheduler with custom cron rules, only predefined presets are supported. For custom rules, use the crontab scheduler.

You won't be able to edit this and will lose your settings if you ever edit and save this. There should possibly be some kind of validation for this, this doesn't seem like a complete fix.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland
πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

Including this in πŸ“Œ Use Gitlab CI Active , you usually can't replace classy with stark, there's a reason tests use classy. the simple replacement for that is starterkit_theme, did that over there.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

Berdir β†’ created an issue.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

The other issue was committed, this seems to have some extra bits, should be either rebased or closed if those aren't needed.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

Closing, the other issue was committed.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

Merged.

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

Berdir β†’ made their first commit to this issue’s fork.

Production build https://api.contrib.social 0.61.6-2-g546bc20