🇧🇪Belgium @daften

Do we need conditional rendering or will the second option also work for D10.1?

Hi @mr.baileys

After 📌 Add gitlab-ci and fix major issues Needs review is accepted, could you create an MR for this change, so it's also checked against the pipeline?

The MR contains the default Gitlab-CI setup recommended from https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr... → with warnings fixed.

What could still be done is disallow failure for some jobs such as PHPStan, PHPCS, ...

daften → created an issue.

This works correctly, no issues found.
Leaving open for future changes.

BramDriesen → credited daften → .

With working patch now

Rerolled patch for 3.x

The problem is in certain situations, e.g. when sending out email, there was an error as the title described: `The controller result claims to be providing relevant cache metadata, but leaked metadata was detected. Ensure you are not rendering content too early`

Updating the title, this seems to be new since Drupal 10.2 specifically

I've updated the MR with a patch that works for me. I made the MenuLinkConfig plugin independent from menu_link_content, which should also fix the todo that was still there.

It's only tested briefly, so please thoroughly. I see e.g. in the menu editing for menu link config items that the machine name is not populated, not sure if that was there before.

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

This works fine for me

Hi GaelG,

I checked, and I can't mark any threads as resolved for some reason, I don't understand why. I checked all threads and saw you checked it already, so I marked the MR as ready, does that help you to move forward?

If you have any idea why I couldn't mark threads as resolved, please let me know, I'm very curious to know if I'm missing something or made a mistake somewhere.

Thank you for the patch. I don't have time to work on the original solution by splitting off pathauto dependent items to a submodule, if somebody feels like doing that, please open a new issue.

I've accepted the patch and will release a new version.

@nightlife2008, please keep in mind that patches should be made based on the git repository, not a downloaded version. Drupal.org adds version information, making the patch not apply fully. It was easy to fix, but could help prevent timeloss for other patches you might create :)

Thanks for the contribution!

I tried to reproduce this problem using the steps from the description, but couldn't do that. Maybe the link being shown is already prevented by another contrib module on the project I tested it on or the behaviour changed in Drupal core.

The patch looks good, but without a means to reproduce it, I can't merge it in.

Thanks for notifying us of the other module. One thing preventing me from marking this module as deprecated, is the lack of security coverage support in Email confirmer. That is quite important for the projects we use this module in.

I can however add a link to the other project on this project's page (which I did).

I will also do a check to see if everything from this module is covered in email confirmer and if so, discuss our next steps internally.

Marking this as postponed until email_confirmer gets security coverage.

Merged, thanks for the contribution!

We needed something quick and reporting API wasn't ready yet. If it makes the csp_log module deprecated, please just let us know, we'll update the docs and mark the module as deprecated then :)
Thanks for picking this up already :)

daften → created an issue.

daften → created an issue.

daften → created an issue.

Works fine for me!

We only need a better solution for the getId method, having an empty id method that should never be used for some valueobjects doesn't seem right. Maybe we can have ValueObjectBase and ValueCollectionBase with a common ValueBase? Or using traits as suggested before?

Marking as needs work based on the feedback!

Looks good to me, I'll give Matthijs a chance to look at it, if he doesn't have time, I'll merge. ping me if it becomes urgent!

Thanks for the updates, some feedback again :)

• Why not add sameValueTypeAs and maybe even getId in the interface, so you can use the interface as typehint where appropriate? Or doesn't that make sense?
• You use ValueAbstract::sameValueTypeAs() in the docblock for \Drupal\flexmail\api\value\ValueInterface::sameValueAs, which should be changed to ValueBase::, or if the comment above makes sense and you agree, to ValueInterface::
• It makes sense about keeping the static classes in there, it just looked a bit weird to have only static classes in there, now it just looks more logical in my mind, since there's other regular methods in there.
• In the constructors for some classes a comment says "The constructor is protected", maybe that should be private instead of protected, since the method is private?
• Keeping \Drupal\flexmail\api\value\Value::validateFieldsPresent in there is ok for me, it was just weird to add a method that isn't used. Especially since that's a method that imo should/could be used in the basic API layer. If you feel it's an added value, feel free to add it back in.

I have the following remarks when reviewing:

• The method \Drupal\flexmail\api\value\Value::validateFieldsPresent doesn't seem to be used at the moment.
• It's also a bit strange that helper static methods are placed in an abstract (base) class. Why is that abstract class needed, shouldn't the helper logic be better off in a Utility class?
• Secondary, Maybe ValueBase is a better name, if a base class is deemed useful to add base logic to in a later phase?
• If there is an interface \Drupal\flexmail\api\value\ValueInterface, I'd expect the base class to implement that interface, since based on the name it seems to be something uniform for all value classes.

The logic looks good in general. Could you check the comments above?

Next to that, is there a blog post or more information that can be shared? Since we'll need a follow-up issue to update the flexmail_webform submodule, since there is logic there that depends on the deprecated methods.

daften → created an issue.

The patch being empty is a mistake on my part. I was trying to provide a fix quickly, but rushed it. Since the error is gone now, I'll close this issue. If it's not ok, feel free to re-open. My fix was in case pathauto wasn't installed, which could still be needed, but the folder in which I created the patch was deleted again.

I've updated our bandaid with a fix that should hopefully fix the breadcrumb issues in general.

I contacted Flexmail and they changed the API so it always accepts an array of strings. They just need to fix the error message which is not correct anymore: `{"title":"Bad Request","type":"about:blank","status":400,"detail":"test_veld should be a string or an array with one string"}`

In attachment a patch that makes sure custom fields work again, together with 2 small changes for checks that should avoid notices in the logs.

Hi,

Please find a patch in attachment that should fix the problem. I didn't have time to test it out myself, but it basically puts all pathauto dependent functionality in a submodule.There's an update hook for existing sites that will install the new module automatically. After you apply the patch and clear the cache, it should work. If not, please report back with the problem encountered.

Hi, Do you have pathauto installed? If so, which version?

Merged, thanks!

For anybody needing a solution, we've developed a small module that takes care of it: https://www.drupal.org/project/path_alias_length →

Any feedback welcome there of course.

Fixed and created a new release. Also changed it a bit to exclude Drupal 10 for 📌 core_version_requirement not recognized before 8.7.7 Closed: won't fix . Thanks for the patch!

Drupal 8 is no longer supported, so fixes for that won't be happening. I'll make sure to exclude Drupal 8 fully in 📌 Drupal 10 compatibility issues Fixed

I am not incorporating any further patches through this issue. Other fixes were done through issues like #3239671: Fixing test in 4.x Branch → and the earlier commit in here. Testing the latest dev version shows no issues by upgrade_status. Any further fixes that are needed should go through different issues, including the cosmetic changes. Those are very welcome of course, but I'd prefer to keep issues centered on their scope :)

There will be a release for select_or_other with D10 compatibility in less than an hour, sorry for the delays!

IMO it would be better to use iconv vs the custom function: iconv('UTF-8', 'ASCII//TRANSLIT', $string);

If you agree, please update the patch. If you don't, can you explain why the custom function would be better?

Merged, thanks for the contribution!

Putting this back to normal priority (see https://www.drupal.org/docs/develop/issues/fields-and-other-parts-of-an-... → .). No data is altered, a blocked user can't use the one-time-login link. As can be seen in e.g. \Drupal\user\Controller\UserController::determineErrorRedirect

daften → created an issue.

I incorporated that contribution in a new release Matthijs, thanks. Putting back to needs work for the edge cases mentioned above.

I created a new MR that uses plugin configuration instead of third party settings. I kept getting errors because the config was stored in third party settings due to checks in \Drupal\ckeditor5\Plugin\Validation\Constraint\EnabledConfigurablePluginsConstraintValidator::validate. I'd also think switching to CKEditor5 would be a good point to store everything correctly :)

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

Patch reroll for the latest version

In our case, the patches that work on \Drupal\path\Plugin\Field\FieldWidget\PathWidget::validateFormElement don't work, because the widget is active. Having the widget active should not keep this bug active. I don't agree Alex, this is not a problem present because of a widget, is an underlying issue in the node_grants system.
The patch from #64 fixes it for us.

daften → created an issue.

The problem is in core and is 🐛 Cannot save translated nodes after upgrading to 8.8 due to invalid path Needs review

daften → created an issue.

Rerolled patch that works on top of 8.x-1.0. The Unicode fixes are already in there, so the patch from 34 didn't apply anymore and the patch in comment 32 still used the Unicode class.

The patch from #32 has one flaw. If the video embed field doesn't have any providers activated, which should allow all of them, no video adding can be done with the form from the patch. The patch fetches the allowed_providers configuration directly. Easy workaround is to enable all providers then.

Correct patch in attachment, apologies

Patch in attachment.

daften → created an issue.

Another update to include not only the ip, but also the user based flood variables for pass reset

Updated patch with better default variables

Patch in attachment

daften → created an issue.

nginex → credited daften → .

nginex → credited daften → .

A new patch file with separate permissions.

IMO the unmanaged files list could be put in the content menu, but that's a separate issue if other people agree it would be better.

Patch in attachment

daften → created an issue.

daften → created an issue.

Thanks for the feedback, but :)

• To which version are you applying the patch? Because the flood_control.ip is a completely new file, it simply doesn't exist in the codebase yet, unless you've already applied a previous patch. We use this patch now on a site with composer-patches on the latest stable version, and that applies without a hitch.
• Valid comment about variables, but I can only spend more time on it if we need a re-roll of the patch. I also think this specific abbreviation (which I took over from the patch from comment 5, I started from there) should not give issues in readability since ctrl is the most common abbreviation for control that exists. If it already happens elsewhere in code, wouldn't it be good to have a separate issue to fix that in general then? :)

Added a patch that lowers the level from emergency to critical. Since this is not a situation that adheres to "Emergency: system is unusable.", but more "Critical conditions.". Since cache invalidation errors can lead to outdated information, i didn't use "Error conditions". If it needs to be higher, I'd propose maximum "Alert: action must be taken immediately".

Updated file, previous one had an error in it's generation

Updated patch with comments from above tackled and some bugfixes.

Next to the comments above, which should be fixed, I've also noticed the unblock form doesn't give accurate information now. The method \Drupal\flood_control\FloodUnblockManager::isBlocked will need to be updated to make sure the blocked status is determined correctly in the unblock form.

Also in the services.yml file '@.inner' doesn't work.

I'll try to update the patch

We still need to make sure the latest version with this fixed does not get installed with older drush versions imo. :)

daften → created an issue.

This was wrong, I misread the output of the composer commands

daften → created an issue.

daften → created an issue.

daften → created an issue.

daften → created an issue.

daften → created an issue.

daften → created an issue.

This was fixed already in https://git.drupalcode.org/project/rocketship_paragraphs/-/commit/decaaa...

Patch incoming soon.

daften → created an issue.