Porta Westfalica
Account created on 9 May 2008, almost 16 years ago
  • Drupal Software Engineer & Developer, Project Management at DROWL.deΒ 
#

Merge Requests

More

Recent comments

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

Also doesn't work for us in Drupal 10.2. Perhaps it's incompatible with Gin Admin Theme?

Saving a node with the button from this module simply does the same as clicking "Save". Any ideas?

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

Anybody β†’ created an issue.

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

@jordan.jamous could you create your patch as MR?

We should also add a test which reproduces the issue (simply checks the log entry for that case) to ensure it's broken without and working with the patch. The steps can be seen above, I think it's jus the case if English is enabled, but not the standard language?

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

@tobiasb based on your comment #5 I assume you mean that there should be a hard exclusion for "en" in code?

So Drupal skips early for cases like
https://ftp.drupal.org/files/translations/all/simple_sitemap/simple_site....en.po.

right?

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

Thanks, could this please be turned into a MR for further review and merge?

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

@doxigo is this still the case in the latest version? If yes, would you prepare a MR?

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica
πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

@mparker17 could you please turn this into a MR?

@Wim Leers, 2Y later still plans for review? ;D (joking, sorry)

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

Thanks, merged into dev!

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

@darvanen +1 on that! Totally agree!
@smustgrave how can we best get a decision here, before wasting time?

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

Thanks @darvanen! I didn't see that one yet!

I copied your fix into a separate MR over here against 11.x and to also make it testable manually using the tugboat link!

My MR meanwhile doesn't seem to fix the issue. Perhaps we need both fixes or just the one in views?

Could you also have a look at #25 please? Similar strange code can be found in views here, near your change:
https://git.drupalcode.org/issue/drupal-2853130/-/blob/c291bb2dafe715cd8...

We should next find out:
1. Is it a views bug or a claro bug? (The svg's & icons seem correct as a starting point) You already gave some information on that
2. At which places does it have to be corrected (see the lines above here)
3. Why these crazy code lines in #25 and above?

So we can decide how to fix and test it properly.

I agree with @smustgrave a issue summary cleanup is also needed, or should we instead reopen your issue (if it's a views bug and not a general / claro bug!)

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

Thranks @johnzzon! I just verified the MR is equal to the patch and it is! Thanks for that on 11.x!

As it was confirmed several times to be working (also by me), I just left one question that's not clear to me code-wise, see the comment.

Also we need a simple test to ensure this works as expected and the class is there in the case, where it wasn't before. Thanks!

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

All good @sachbearbeiter you're totally right. It's just a heavy refactoring required and the world turns fast...

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

Sadly yes. COOKiES 1.x implementation / use of config translations is... well... let's say not the Drupal way.

We were planning a rewrite with COOKiES 2.x but that's a bunch of work and won't happen without sponsors, I think. We already put a lot of effort into COOKiES as a small team of 3 developers... paid by our spare time.

Sorry, but feel free to help.

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

Forgot to create the MR from the issue fork, sorry! Hiding the outdated patches.

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

Ok here we go.

I'd be interested why someone wrote this curious line without even leaving a comment on this crazy confusion:

$context['sort'] = (($context['sort'] == self::ASC) ? self::DESC : self::ASC);
πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

The icons are already correct, just the classes are the wrong way around:

  • tablesort tablesort--desc => tablesort tablesort--asc
  • tablesort tablesort--asc => tablesort tablesort--desc
πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

This is really funny... this bug seems to exist since Drupal 6 and the argumentation is

It's not wrong. It's saying what will happen when you click on it. Not what is currently happening.

( #552752: hover text and file names are incorrect for arrows indicating ascending and descending sorts on table columns β†’ )

It's true but well, everyone is doing it the other way around and that's typically what people are used to ;)
I can't believe Drupal is doing this differently from all other software sorting UI's I know! :D

Some for reference:

Just count the stars... what else has to happen to accept it's the wrong symbol in Drupal, indicating the current sorting order? ;)

  • If the arrow is going up on a sorted row, the results are expected to be sorted ascending
  • If the arrow is going down on a sorted row, the results are expected to be sorted descending

These screenshots are from claro, but it's the same in Gin for example:

(simplytest.me Umami Demo)

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

Currently Proposed resolution says:

Probably the best solution is to clear out this content key and automatically put all fields into hidden when LB is enabled.

I'd like to add considering to also prevent rendering the fields present in "content" when layout builder is enabled to ensure they're not handled in the background, even if present due to any reason. If that's not possible, we should show a warning in layout builder and allow to clear them in UI? (Hopefully that's not needed!)

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

We hit this once again! This often seems to happen in the background (which is bad for performance) and is getting visible when causing issues like "Recursive rendering detected when rendering entity…" in our case, for a field, which isn't used at all in this viewmode.

As the issue describes correctly, that field is being rended, because it's
part of "content"
but not part of "hidden" and "third_party_settings.layout_builder.sections"

That means it's handled & rendered by Drupal (unexpected), but not visible (expected)!

Setting the priority to "Major" for the performance reason and the reason that bugs caused by this can only be resolved by experts in config.

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

@AndyF: Indeed that should go into a separate issue. But you may want to set this issue related and vice-versa.
40,508 sites report using this module, so I think it's important to check that.

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

@trebormc please try contacting a maintainer

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

Same issue here, confirming #12. Are the maintainers aware of this?

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

Thank YOU @catch! Great to see this fixed finally :)

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

I added it separately as #suffix now, think that makes most sense.

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

@thomas.frobieter please review and try.

Regarding the abbreviation: There isn't really one in English:
https://www.quora.com/What-is-the-correct-abbreviation-for-monthly

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

Too much work for now. Maybe one day... good enough so far.

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

@thomas.frobieter I added another MR which sets the #wrapper_attributes instead of #attributes as I think that is what you were looking for!

This is the result:

<div class="active js-form-item form-item js-form-type-radio form-item-planvariant js-form-item-planvariant form-disabled">
<input data-drupal-selector="edit-planvariant" disabled="disabled" type="radio" id="edit-planvariant-zmp-live-plus-bronce-yearly" name="planvariant" value="zmp_live_plus_bronce_yearly" checked="checked" class="form-radio">

Or should we probably set both for maximum flexibility?

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

@thomas.frobieter please review.

The result is like this then:
<input class="active form-radio" disabled="disabled" type="radio" id="edit-planvariant-zmp-live-plus-bronce-yearly" name="planvariant" value="zmp_live_plus_bronce_yearly" checked="checked">

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

Okay this works, but it would be great to track this down to the twig code / file where this is wrong. Any chance for that? Ideas?

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

As the current implementation in twig_tweak is quite simple (render array), we'd have to duplicate some of the logic to explode $id by "|" and ":" so it's to be discussed, if it wouldn't be even better to add that check to core to safeguard the render array in general, even if used somewhere else.

Current twig_tweak implementation:

  /**
   * Builds contextual links.
   *
   * @param string $id
   *   A serialized representation of a #contextual_links property value array.
   *
   * @return array
   *   A renderable array representing contextual links.
   *
   * @see https://www.drupal.org/node/2133283
   */
  public static function drupalContextualLinks(string $id): array {
    $build['#cache']['contexts'] = ['user.permissions'];
    if (\Drupal::currentUser()->hasPermission('access contextual links')) {
      $build['#type'] = 'contextual_links_placeholder';
      $build['#id'] = $id;
    }
    return $build;
  }
πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

Just ran into this again in a different project. wd_ is our table prefix.

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

Happy to see this RTBC'd! Should the version here stay 11.x or be set to 10.3.x or even 10.2.x now with https://www.drupal.org/about/core/blog/drupal-11-is-now-open-for-develop... β†’ ?

This should go into 10.3.x at least, 10.2.x if possible as it affects many use-cases and batch operations.

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

Just ran into this when indexing rendered content using search_api (indexing nodes with media entities): #2913931: Recursive rendering detected when rendering entity β†’

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

@Chi sorry false alert, something seems to have gone wrong on our side. All good, code looks fine! The error didn't make sense code-wise.

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

Sorry @Chi yes, I was actively working on it, just wanted to create the issue first. But seems like false alert, something seems to have gone wrong with composer patches. I deleted the module and re-downloaded it via composer, not the error is gone. Sorry!

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

The question is, how to solve this properly and with most flexibility. We may compare this to similar modules and check if "Author" should be a special setting.

I'm not really sure yet, what's the best fix here.

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

I don't think "Inherit from parent" is what this module is supposed to do. But it "parent" is just an entity reference from the "child" it should work already. Then it might be a different issue, but "parent" is a bit misleading in this context.

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

I guess this might be related to πŸ› Incompatible with Entity Type's ::checkAccess() implementations Active

Which settings are oyu using?
Which value are you using for the "If the conditions do not match:" setting especially?

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

In the first step we need tests to clearly reproduce the situation in code. Assigning @Grevil for this and to take a general look. Read the other issue and the information above carefully first.

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

Okay I think this is still a highly relevant feature for non-solr Search API configuration

It's absolutely typical to boost more recent content, but the workaround here isn't a proper fix. With the available number based boosting options you're able to boost the timestamp, but the resulting boost value is something like:

1708939471 * 1.0 = Boost: 1708939471
1708939471 * 1.1 = Boost: 18798334181
and even
1708939471 * 0.1 = Boost: 170893947.1

So I think we need a solution similar to what search_api_solr provides: https://www.drupal.org/docs/8/modules/search-api-solr/search-api-solr-ho... β†’ for the "created" and "updated" fields. Any ideas? I'm sadly not very experienced in this topic :/

Hope it's okay to set this feature request to major, as it's a really typical and relevant use-case in my eyes.

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

Thank you very much @geek-merlin that looks super interesting! :)
I'm done with my project where I ran into this, but will definitely consider it in the future!

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

Thanks @juan1975 if you or anyone else is able to reproduce this, please reopen.

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

Any chance to get this committed and released?

The issue still exists... just ran into this again. RTBC since > 1y

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

@Grevil: I guess we should close this won't fix?

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

Thanks @cbccharlie while I don't think we should switch to a hook generally, I'm totally fine with providing better API ways to use COOKiES like this, if there are no other downsides :)

I commented the other issue.

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

Thanks @cbccharlie nice work! Green lights from my side, this totally makes sense code-wise! :)
Now this needs a rebase and someone should ensure that this doesn't cause regressions or side-effects (while I don't expect them).

Thank you! I think we should get this merged soon.

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

@ressa: Thanks, yes such a switch or any other global option to opt-in to CDN or being able to disable it would be fine. It's really dangerous for GDPR reasons, but I can see why developers outside the EU don't care that much.

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

What do you mean? The other one also has a MR.

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

Thanks for all the feedback here. I think all makes sense. Introducing HTMX in Drupal is not a simple task and will require many resources.
So starting in contrib, making concepts and keep an eye on the general adoption of HTMX is surely the right way to go. Especially we should have a look at DX improvements. I agree that the current AJAX system in Drupal works, but for me it always has been and still is kind of (wonderful) magic in many parts. I agree with @catch in #17 and saw many developers struggling with it. It's not really straight forward, but that doesn't mean it's not good!

So I think we're at a point, where things work well, but it doesn't mean it couldn't get better. Especially for developers new to Drupal it might be a simplification and unification, similar to what happened with Symfony in Drupal 8 in contrast to the custom codebase in Drupal 7, that also worked great, but was a good choice to use a standard library to make the switch to Drupal easier.

So there are many good reasons for and against HTMX and the decision has to be made wisely. Trying it out in contrib is a good next step and perhaps someone would like to research the general pro's and con's and feedback for HTMX?

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

Any ideas how we can get more information about the root cause?
May this be a single malformed (file) entity? I see some file entities without type in my file list (fonts and csv's).

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

I'm running into this after updating to 7.x-2.38. Still unclear, why it happens in my case...

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

Hi @neptune-dc and thanks for your detailed report!

I looks like it's a duplicate of πŸ› Aggregated JS knocked out Needs review ?

The strange thing is, that these reports are popping up now. Perhaps related to a core change? I guess we should close this issue in favor of the other one?

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

@code-brighton meta-answer: Did you ask Chat-GPT perhaps? Seems this would be a good idea ;) (but check the result)

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

Thanks @bgreco that looks great!

Wouldn't it perhaps make sense to test both: With and without aggregation enabled? So we should perhaps better have 2 tests calling a helper method?

    // Test that scripts are knocked out even when JS aggregation is enabled.
    $this->config('system.performance')->set('js.preprocess', TRUE)->save();

@Grevil what do you think?

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

@bwtechuk maybe a replay attack, I think.
Which captcha type are you using?

You could also try https://www.drupal.org/project/friendlycaptcha β†’ or https://www.drupal.org/project/recaptcha β†’

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

FYI if it helps others: We finally decided for https://www.drupal.org/project/ui_styles β†’ and against this module as it's more actively maintained in general and also a nice concept. Of course that shouldn't mean, this module isn't great! :)
The other one was just also good and the safer choice for us.

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

Thanks @tunic - I agree in many points. But adding another setting / config would make things even more complex, so I'm not a fan of that.

@Grevil what do you think?

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

@smustgrave thanks for the ping, I didn't forget this, but currently can't take the time. Open for anyone, sorry.

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

Token support is not a bad idea, but tbh we never needed it. So feel free to implement this as optional feature in a submodule here as merge request. Thanks! :)

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

Thanks @bgreco! Good finding! This makes sense to me, but I'd like to ensure it works using FunctionalJavascriptTests.

Could you please add a simple test, based on the existing, which fails with the current implementation and succeeds with the new one?
I'm also fine to test, if the script is present separately or something like that.

@Grevil: Could you perhaps test this manually by time?

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

(but I'm not generally against adding this permission - still it might conflict with that module if already in use)

To not make a breaking change, this would need an update hook to give the permission to at least inform users about the change.

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

@CRZDEV thanks. I think instead it would make sense to use the field_permission module?

https://www.drupal.org/project/field_permissions β†’

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

Thanks @Berdir for the wakeup call - sorry I didn't think about that -.-

I agree with @Grevil that I might not be the only stupid person running into this and we should perhaps at least for the defaults consider to explicitly end the word after .asp (especially because .aspx exists), while I'm not totally sure about the .php cases.

What would you prefer?

Additionally, it might make sense to at least add a note to the description about such cases, because not every site maintainer is a developer? Should we ask someone else for opinion?

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

Just took a look and LGTM! I'll ask @Grevil to review it finally. Thanks @Berdir, great work as always!

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

@juan1975 did you perhaps forget to select the appropriate text filter you configured?

Honestly, I have no clue why it doesn't work for you. I guess you'll need to debug it deeper on your side. Please let us know, if it's a bug in the module, but I don't think so, sorry.

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

@pcambra perhaps try contacting the maintainers? They don't seem very active.

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

Does the Commerce team agree? Then we'd be willing to implement this as MR.

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

Thanks for your investigations @dinazaur! Or should we simply close it and leave things as-is? What would you suggest?

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

Thanks @mlncn you're right, the documentation should be updates on the module page and the README.md to reflect the latest possibilities and settings. Could you prepare something perhaps?

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

Thank you @mortona2k LGTM!

We could consider making it "#open" => FALSE by default, but that's preference and until now it was open. So let's keep it like this for now.

πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

I'd see this as bug. The question is, if we should add the update.php by default to the list or exclude these urls in the background... but tend to add it by default, so it's visible to users and straight forward.

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