🇩🇪Germany @lrwebks

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

All comments solved. @jurgenhaas, would you mind reviewing once again?

@anybody, yes. The plugin is there now, but I still need to trigger the CrowdSec signal when the user fails a login attempt. If there are other events that should be reported to the CrowdSec module, let me know and I will add them as well.

Okay, @anybody and I opted for a simple module exists check that runs in the background. Simple but effective!

Fixed your two comments and added the README section, @anybody. Unfortunately, I cannot resolve the threads as I am not maintainer of this module.

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

@anybody, would you say that this is more of a checkbox or more of a submodule kind of thing? I'd opt for a small submodule titled someting like "login_security_crowdsec" but I can understand if a submodule just for that may be unwanted by the maintainers.

Static patch until this is merged

Static patch until this is merged

Static patch until this is merged:

Changed the confirmation message to your suggested one!

@danrod, definitely hope you get well soon! @anybody assigned this task to me in the meantime, hope you don't mind!

Implemented! Attached a screenshot of what the warning message looks like. If the promotion is not used, the default Drupal message is shown instead.

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

LGTM!

Indeed, 📌 "WatchdogMailer" logger runs too early for @date.formatter to be injected, causing a fatal error on cache invalidation. Active broke the limit mail system, because apparently, UNIX timestamps need to start with an "@" symbol when creating datetime objects (not a very great standard in my humble opinion). Tests should be green now and monkey testing also shows that it works!

The tests are correct in saying that the limit mail is not sent anymore, and the pipeline apparently didn't fail before 📌 "WatchdogMailer" logger runs too early for @date.formatter to be injected, causing a fatal error on cache invalidation. Active , which changed something in the limit mail behavior. So, I'm pretty certain now, that that issue broke the limit mails. I will see what I can do.

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

I have added a basic functionality test now, unfortunately the JSON:API pages always result in a 404 and I haven't quite figured out why yet.

Currently, 11.x-dev is broken because of this problem, but the current solution in here is working correctly. We currently don't have any tests in place *at all* for this module. Should we implement one for this here?

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

@anybody: Do you remember what we wanted to change here? I understand that we do not want to change the GET request with query parameters, but I currently don't see any other way to provide all those specifications for what we want to the controller.

Everything implemented and working properly now!
I'll attach a screenshot of how the error currently looks.

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

Okay, so the simple approach gives us our expected behavior, but it has now broken a different test. In that test they seem to use the PlainTextOutput::renderFromHTML() function on “half” of a tag and then some connected tags. Since PHP strip_tags does not strip the half-tag, we get a whitespace in front of the <script> tag.

@anybody, any suggestions on what to do regarding that? I can't think of a simple way to handle this exception (but handling that should be addressed either way, in my opinion).

I have implemented a simple but in most cases effective approach and I hope that it will work once the test is done!

Note that the test I have added is supposed to fail, with the exact reason that it is showing right now:

Failed asserting that two strings are identical.
Expected: 'Giraffes·and·wombats'
Actual: 'Giraffesand·wombats'

This shows that the problem does exist and is not fixed until this test runs fine.

@anybody, do you think we should still implement the “simple approach”, since it is likely going to get rejected anyway for not being robust enough? Or is it a good proof-of-concept?

lrwebks → changed the visibility of the branch 11.x to hidden.

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

Done, now it works just like the “Media” module does!

• Add advertisement link
• Special links to add advertisement of specific bundle
• Add advertisement type link in structure menu

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

FYI: This function is also used in contrib, e.g., in the Metatag → module to strip HTML tags from token values that are later put into <meta> tags.

We came across this when we realized that this function does not leave spaces if two HTML tags sit next to each other, and the resulting text is then missing a space. But I suppose as long as this function is not regarded as “safe to use” for HTML stripping, we should switch to something else? Are there some viable core alternatives (instead of a DIY solution)?

Okay, the problem seems to stem from the renderFromHTML() method of the Drupal helper class PlainTextOutput. It internally uses the PHP strip_tags() function and then decodes remaining HTML entities. That PHP function doesn't care for any spacing when removing the tags, so the result is missing spaces.

So, this is technically not a Metatag fault. But perhaps we want to solve this issue here anyway? Otherwise, we might push it up to Drupal Core (but I doubt that this will be classified as “wrong behavior” there).

I will still write a test to ensure the HTML stripping works correctly, since I could not find one regarding that functionality.

@anybody and I are currently working on a fix that does not change the interface of the GET request too much, if at all.

Works at least for me!

lrwebks → created an issue.

In general, we should perhaps find the cause of why the impression ID was null in the first place. @dobe right now, our solutions are equally correct, but let me take a look at what is actually happening under the hood there.

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

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

Code-wise, this also looks good to me!

Re #4 by the way: The solution you have implemented is totally understandable to me and I can definitely grasp the fetch API. But converting that myself would have certainly wasted more worker hours than it should! So, glad that someone else took care of it.

@anybody: Tested it with multiple different ad types and it works as expected! Even the problem I had personally, where the ads would only display for regular users and not admins, seems to have resolved itself. That leads me to believe it might actually have been a JQuery quirk as well… Who knows?

Now every submodule has a proper README and a help hook. Since I am not really involved with the concrete functionality of all those modules, please remind me of any important steps that would fall under the “Configuration” section or add them yourself if you have the time. Thanks!

I've run into a bit of a problem just now:

Usually, when trying to translate standalone config entities (not attached to any other entity or something similar), all edit forms of that entity will have the same route name (e.g., entity.my_config_entitiy.edit). You can then use that route in the config_translation.yml and the work is done.

This module however (and I think that this is custom coded) modifies the route names of the extra field entities to signify what entity they are attached to (e.g., entitiy.some_base_entity.entity_extra_fields.edit). That makes translation impossible (in a simple fashion) as far as I know. Why? Because the config translation system is build upon the former of the two approaches and expects static route names (no wild cards) in the translation routes within config_translation.yml.

It might still be possible to solve using code and not YAML, but I was unable to find any suitable approach online and via AI, and for that matter, any approach at all.

@anybody: What shall we do now?

Done!

As just discussed, assigned to @anybody.

Noted and moved up my list!

Alright, in that case we should probably prevent the user from adding more than one different paragraph type to the form field. That would be my first thought on a possible solution at least. We somehow have to prevent that buggy result, but I can totally understand if we so not want to undermine the module's design here.

That is correct, but the paragraph field settings allow to select / filter multiple paragraph types. In that case, using the “Paragraphs Table” formatter on the form view should prevent any other paragraph types from being added. (The view formatter actually works well, as it creates a separate table for every type added to the paragraphs field, so why not just change the type filtering behavior and call it a day?)

As it stands right now, a user is able to evoke undefined behavior in the module without changing code, and that is a bug, regardless of the module's purpose or design.

That said, we would nonetheless be eager to propose a solution to this as soon as we have the time. :)

lrwebks → created an issue.

@anybody, I think that I might be misunderstanding what you want from this issue. Do you want the extra fields to be translatable within their config (on the “Manage extra fields”) page, or within a content entity form (when editing a content entity with extra fields)? As far as the second one goes, I don't think that there is a universal option for doing this with every entity, because this requires the entity to be translatable as well. Also, I'm a bit confused since you appear to want the second solution, but your talk about a config_translation.yml seems to lean toward the first solution.

TL;DR: Could you clarify what you want to be translatable specifically?

It currently appears to be near impossible to change the field description in the alter hook, as every attempt I have pursued at changing it seemed to have no effect. I have committed my best attempt at this time, which successfully updates the description in code, but does appear to cause any change in the displayed text.

@grevil, perhaps you know a proper solution to this?

I'll quickly add a conditional message if the user is lacking the permission to edit the JS field.

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

Definitely a necessary fix as the namespace is wrong. So I added that to the MR. But unfortunately I cannot reproduce your issue. Can you try again with the issue branch and tell us, if the problem is gone now? In that case, fixed!

lrwebks → created an issue.

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

Definitely works, but TRUE is more appropriate here as the array value. It doesn't set the attribute's value to “TRUE” but rather adds the attribute like a flag attribute without the empty string value. I will change that quickly.

All D10 versions are now removed from the info.ymls!

Works as expected now. The additional check with the URI is necessary, as the link module shows the link URI as the fallback link text when either nothing was entered in the entity form or the link text was disabled in the field settings.

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

@anybody: +1 for that from me! So should I remove the versions from the info.yml in here, or should we close this as outdated?

@socialnicheguru: Okay, tested it on 10.5.1 and getting a similar error after installing ad_content with the same cause as your issue. Yes, in Drupal 10.5.x there is no attribute class for content and config entity types yet, only annotations. Meaning: Your patch is technically the correct and only solution to the problem.

However, I really am against removing the new attributes in favor of this issue. @anybody, what do you think? We are now faced with the decision of removing progress or removing backward compatibility... What is the best step here in your option?

I currently cannot reproduce the WSOD with the newest development version on the 11.x branch. @socialnicheguru, if you haven't already, could you also try again using the newest dev version to see if we have (accidentally) fixed it in the meantime? Also (if it isn't too much effort), could you try the installation on 11.x as well, to see if it is a Drupal 10 problem? Thank you in advance!

The patch provided is not the solution that we want. Simply reverting the new Attributes to old Annotations is a step backward (and backward is always the wrong direction. :)) I'll see if I can reproduce this, since I never ran into that issue before.

It is always optimal to provide the changes made for this issue in an MR, so I will do that quickly and then take a look.

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

Everything is done now, apart from finding the reason of the test failure. But as far as I understand, @grevil will take a look at that? So I'll set this to review now.

As discussed with @anybody, I have moved all code related to the JS ad type into a separate submodule, and we will tackle the proposed solution there.

lrwebks → created an issue.

lrwebks → created an issue.

Added that, as well as some other things that I remembered, that should probably be in the README.