🇧🇪Belgium @dieterholvoet

Yes, it looks like negotiating the active domain this early in the request (during the loading of config overrides) is not supported. I started a MR with an approach where the active domain is loaded on demand. Having our own static cache is unnecessary since the active domain is also just a property on the DomainNegotiator service, so there isn't any performance gain.

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

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

I also added a custom translation handler to actually make publishing of translations work. The code is mostly based on Drupal\node\NodeTranslationHandler and Drupal\eck\EckTranslationHandler.

I don't think it's necessary to add a custom setting to control the visibility of the status field. That's what the form display UI is for. I'll change it so the field is hidden by default and can be displayed by moving it in the form display. I also changed the default value of the publishing status to TRUE.

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

I created a new MR targeting 3.0.x.

dieterholvoet → changed the visibility of the branch 3090261-setinlineblockdependency-override-might to hidden.

dieterholvoet → changed the visibility of the branch 3090261-2_15 to hidden.

I started a new MR on an issue branch instead of the main 8.x-2.x branch.

dieterholvoet → changed the visibility of the branch 8.x-2.x to hidden.

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

dieterholvoet → created an issue.

dieterholvoet → created an issue.

dieterholvoet → changed the visibility of the branch 1.x to hidden.

Is the idea of preprocess hook because we don't have a single spot in the plugins/classes to build them?

Yes, just to make it work anywhere #theme => footnote_links is used. I guess it's only used at one place in the module, but it could also be used in another module. And also to support controlling this behaviour by passing #simplify_sequential_citations => TRUE/FALSE.

Are you sure they are actually still required? The red asterisks still shows, but you shouldn't get any validation errors when saving the entity.

Maybe also update all existing redirect entities in a update hook to stop using any of the removed status codes?

dieterholvoet → created an issue.

This InvalidArgumentException is also being triggered by bot traffic on all our sites using the Lazy module because it uses this RequestPath condition plugin. It's being triggered by visiting the path /index.php.suspected, so not just paths with double slashes. That's why I want to propose to also just catch any InvalidArgumentExceptions thrown in the getAliasByPath() method.

Never mind, that doesn't fix it. I now have citations that link to #footnote1, while the footnote has an ID of footnote1-0. That's on a page with multiple text areas with footnotes and with all of them aggregated at the bottom of the page.

dieterholvoet → created an issue.

For me this fixes the issue where links from citations to grouped references at the bottom of the page weren't working anymore.

These two .env variables are specific to the Laravel integration of Ignition (source), there are not (yet) supported in the Drupal module. Currently, the only way to change these settings is through the UI. I'm changing this to a feature request.

Why not postpone adding the argument to the interface until the next major version and until then make it an optional argument in worker classes like this:

public function processItem($data, ?object $metadata = NULL): void

calling the method like this:

$worker->processItem($item->data, $metadata);

That way it works both for workers with and without the method and the necessary changes to queue workers will be a lot more straightforward.

At some future point we could deprecate the processItem() method and just support the new method and interface?

If you're going to eventually do a breaking change, might as well stick to the same interface and method name and make the breaking change the addition of the new argument to the existing method, no?

dieterholvoet → created an issue.

dieterholvoet → created an issue.

Do we really need a new interface and method here? Why not just add the metadata as an optional argument to the processItem function and add it to the interface in a new major?

dieterholvoet → changed the visibility of the branch 3430646-automated-drupal-11 to hidden.

dieterholvoet → changed the visibility of the branch fix-composer-json to hidden.

Thank you! I opted the project into security coverage, added Gitlab CI, finished and merged all issues and created a first release. I'll stay around for any new issues as well, for the time being.

dieterholvoet → created an issue.

Can't seem to reproduce this in the latest version.

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

dieterholvoet → changed the visibility of the branch 3391771-balloon-panel-is-misplaced to hidden.

I tested that and yes, that works too! Thanks for the suggestion.

Just want to plug Dead Letter Queue → here, which solves this problem in contrib.

I sent the following message to s_leu and tim bozeman through their contact form:

Hi,

Could you post a comment on my offer to co-maintain CKEditor ID Attributes ( https://www.drupal.org/project/ckeditor_id_attributes/issues/3519478 💬 Offering to maintain CKEditor ID Attributes Active )? I would like to become a co-maintainer of the module to help resolve any pending and future issues.

Thanks in advance,
Dieter Holvoet

dieterholvoet → created an issue.

dieterholvoet → created an issue.

Examples still need to be updated.

That seems to fix it, thanks!

Yeah, you're right. Let's just not do this then. Don't you agree it's a bit unnecessary to try and hide the password on a non-public, admin settings form?

Should be possible if you grant the role the permission to view revisions. That permission only applies to nodes the user already has permission to see.

Thanks for your input! You're right, probably not the best idea. It was worth considering though :).

We'll have to drop support for PHP 7.3 if we're adding property types, but I think that's acceptable. Left some comments in the MR.

dieterholvoet → created an issue.

CKEditor 45.0.0 has been released with support for full screen mode: https://github.com/ckeditor/ckeditor5/releases/tag/v45.0.0
I opened 📌 Update CKEditor 5 to 45.0.0 Active to update CKEditor in core.

dieterholvoet → created an issue.

dieterholvoet → created an issue.

The changes in this merge request break backwards compatibility. If someone configured an ignore list with paths with language prefixes, those rules won't work anymore. I think it would be better to check both with and without language prefixes, to make sure we don't break any existing setups. Or we could change the ignore list in an update hook to remove any language prefixes from existing rules.

I tested adding aliases to the ignore list, that works as it should.

That's weird, I have a website using Domain Access, but I can't reproduce this issue. When I attempt to open the node as a non-authenticated user on domain B, it just redirects to the login form on domain B.

mparker17 and louis-cuny seem to be the only active maintainers in the past 2 years. Only mparker17 has their contact tab enabled, so only sending him a message for now. This is the message:

Hi,

Could you post a comment on my offer to co-maintain Structure Sync ( https://www.drupal.org/project/structure_sync/issues/3517380 💬 Offering to maintain Structure Sync Active )? I would like to become a co-maintainer of the module to help resolve any pending and future issues.

Thanks in advance,
Dieter Holvoet

dieterholvoet → created an issue.

Same question as solideogloria, any reason Drupal\Core\Queue\QueueFactory doesn't implement the new interface?

dieterholvoet → created an issue.

Yeah, ideally we would generate entity type classes implementing only the necessary interfaces, like the Trash module generates entity storage classes, but as long as that's not completely necessary I'd like to avoid adding that complexity to the ECK module.

I just thought about something. Most getters/setters in Drupal\eck\Entity\EckEntity have a hasField() check for this exact situation, while the isPublished() / setPublished() / setUnpublished() methods don't. I'll fix this issue that way. Thanks for your input berdir.

Fixed, tests are passing.

Seems like adding those uidowner keys created some new expectations, we have a failing test:

SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'uid' cannot be null

Don't think this really needs additional tests.

If you implement EntityPublishedInterface then isPublished() and setPublished() are expected to work.

ECK allows for entity types to optionally enable the published field, but all ECK entities share the same base class. Drupal\Core\Entity\EntityPublishedTrait::publishedBaseFieldDefinitions() checks both the presence of the interface and the presence of a published key, so we should probably do the same here. That, together with 🐛 Entity keys should only be added to an entity type if they are enabled Active , would fix this issue. I'll work on getting that fix in ECK first.

dieterholvoet → created an issue. See original summary → .

dieterholvoet → created an issue.

The module doesn't let you manually create settings entities, it creates 1 entity automatically. You might still be able to manually create entities with user 1 since that user bypasses all access checks, but there isn't much we can do about that.

I think I found a fix, could you test the changes from the MR?