Account created on 2 February 2021, over 4 years ago
#

Merge Requests

More

Recent comments

🇩🇪Germany Grevil

grevil created an issue.

🇩🇪Germany Grevil

Ok, found one more D11 incompatibility issue. Also, some weird "negate" behavior, but this should be fixed in a follow-up issue.

Please review!

🇩🇪Germany Grevil

Funny thing is, dev is already d11 compatible: https://git.drupalcode.org/project/jsonapi_role_access/-/commit/512c00af....

But there never was a release.

🇩🇪Germany Grevil

grevil made their first commit to this issue’s fork.

🇩🇪Germany Grevil

Static patch for the time being.

🇩🇪Germany Grevil

That should be it, I couldn't find any incompatibilities.

Additionally to the rector patch, I removed the D9 compatibility.

Please review!

🇩🇪Germany Grevil

grevil made their first commit to this issue’s fork.

🇩🇪Germany Grevil

Thank you, @smustgrave! :)

Could you tag a new release? Would appreciate it!

🇩🇪Germany Grevil

All done, please review!

🇩🇪Germany Grevil

Parent issue is fixed now! I'll create the needed changes. Afterward, we should wait for the release.

🇩🇪Germany Grevil

Sorry, wrong issue.

🇩🇪Germany Grevil

grevil changed the visibility of the branch 3548238-broken-when-using-aggregation to hidden.

🇩🇪Germany Grevil

Maybe we should follow what the schema has where body is attached to the text plugin

The MR should have reflected that already, but it didn't. Now it should be correct.

🇩🇪Germany Grevil

Interesting, same pipeline failure as in 📌 MenuLinkContentAccessControlHandler does not allow "view" access without admin permission, making these entities inaccessible via REST, JSON API and GraphQL and entity reference fields Needs work : None of the tests fail, but the pipeline still says "failed".

Setting back to "Needs review", as all tests succeed.

🇩🇪Germany Grevil

I updated the issue summary. None of the tests fail, but the pipeline still says "failed" no idea why. Setting back to "Needs review".

🇩🇪Germany Grevil

Hm there is no "clearCache" on the token service. only "resetInfo" and I am not 100% sure it does the same thing.

Furthermore, there is no replacement defined in this CR: https://www.drupal.org/node/1973488

Since we are already using "token_clear_cache()" inside the normal "TokenCustomForm", I'd say we merge this as is. If it causes any trouble in the future, we can look further into it.

🇩🇪Germany Grevil

grevil changed the visibility of the branch 3531812-deleting-custom-tokens to active.

🇩🇪Germany Grevil

grevil changed the visibility of the branch 3531812-deleting-custom-tokens to hidden.

🇩🇪Germany Grevil

Wow, who though of that! 😅

Ok, my apologies. LGTM then!

🇩🇪Germany Grevil

Although the changes do not match the Issue-Summary:

I have some user roles that I would like to be able to view the overview page, but not add, edit, or delete.

But the code:

entity.token_custom.collection:
  path: '/admin/structure/token-custom'
  defaults:
    _title: 'Custom tokens'
    _entity_list: 'token_custom'
  requirements:
    _permission: 'administer custom tokens+access custom tokens overview'

says that the user needs both permissions (add + edit + delete + overview access) to see the collection.

Either update the issue summary or the code.

🇩🇪Germany Grevil

Tests don't need any changes, they were broken before and just got fixed.

LGTM!

🇩🇪Germany Grevil

@anybody, yea I think that's better. Especially since "token_clear_cache()" is defined in the token contrib module.

But "token_clear_cache()" is also used in other places inside token custom... makes me wonder, whether this module is even compatible with tokens core. Probably not.

🇩🇪Germany Grevil

grevil made their first commit to this issue’s fork.

🇩🇪Germany Grevil

grevil made their first commit to this issue’s fork.

🇩🇪Germany Grevil

Thanks for the patch! Code LGTM and works as expected.

Test failures are unrelated.

🇩🇪Germany Grevil

Ok, I found the reason why the tests fail.

Inside most Functional, JS Functional and Nightwatch tests, the front page is configured as "user/login". Now if we use Methods like "Url::fromRoute('user.login')" we won't get "user/login" returned anymore, but instead we'll get "/".

IMO, this is fine and expected, but of course all the tests need adjustments to actually succeed now.

We have several ways to fix this issue:

  • Define another front page on setup for all failing test classes. (Preferred)
  • Globally, define a different front page.
  • Adjust the tests to use other urls than "user/login" to test their functionality.
🇩🇪Germany Grevil

Ok, I see.

🇩🇪Germany Grevil

grevil created an issue.

🇩🇪Germany Grevil

Static patch for the time being.

🇩🇪Germany Grevil

grevil created an issue.

🇩🇪Germany Grevil

Done! Please review!

🇩🇪Germany Grevil

LGTM!

🇩🇪Germany Grevil

Done, please review!

🇩🇪Germany Grevil

grevil created an issue.

🇩🇪Germany Grevil

Done, please review!

🇩🇪Germany Grevil

If you're not totally against it, @smustgrave, I'd create an issue to give the regular separation a try and we can simply check the results in a MR?

🇩🇪Germany Grevil

Tugboat integration working and Tests are green! Merging.

🇩🇪Germany Grevil

grevil created an issue.

🇩🇪Germany Grevil

This is also reflected in the already existing schema:

tour.tip:
  type: mapping
  label: 'Tour tip'
  mapping:
    id:
      type: machine_name
      label: 'ID'
      constraints:
        # Tour IDs also allow dashes.
        Regex:
          pattern: '/^[a-z0-9_-]+$/'
          message: "The %value machine name is not valid."
    plugin:
      type: string
      label: 'Plugin'
      constraints:
        PluginExists:
          manager: plugin.manager.tour.tip
          interface: '\Drupal\tour\TipPluginInterface'
    label:
      type: required_label
      label: 'Label'
    weight:
      type: weight
      label: 'Weight'
    position:
      type: string
      label: 'Position'
      constraints:
        Choice:
          - auto
          - auto-start
          - auto-end
          - top
          - top-start
          - top-end
          - bottom
          - bottom-start
          - bottom-end
          - right
          - right-start
          - right-end
          - left
          - left-start
          - left-end
    selector:
      type: string
      label: 'Selector'
      nullable: true
      constraints:
        NotBlank:
          allowNull: true

tour.tip.text:
  type: tour.tip
  label: 'Textual tour tip'
  mapping:
    body:
      type: text
      label: 'Body'
🇩🇪Germany Grevil

@smustgrave, if you agree, I can fire up a quick MR.

🇩🇪Germany Grevil

grevil created an issue.

🇩🇪Germany Grevil

grevil made their first commit to this issue’s fork.

🇩🇪Germany Grevil

Please review!

🇩🇪Germany Grevil

grevil created an issue.

🇩🇪Germany Grevil

grevil created an issue.

🇩🇪Germany Grevil

grevil created an issue.

🇩🇪Germany Grevil

All green now, finally! Ready for final review!

🇩🇪Germany Grevil

That should be it! Please review!

🇩🇪Germany Grevil

grevil created an issue.

🇩🇪Germany Grevil

We have a limited amount of time to invest in this module. If the community is willing to create an MR to fix this issue, which is then approved by the community, we could take a look.

🇩🇪Germany Grevil

We have a limited amount of time to invest in this module. If the community is willing to create an MR to fix this issue, which is then approved by the community, we could take a look.

🇩🇪Germany Grevil

Please review!

🇩🇪Germany Grevil

grevil made their first commit to this issue’s fork.

🇩🇪Germany Grevil

@yospyn, thank you for the report!

Pretty basic fix, someone made an empty check instead of checking for null. For 0 empty would be true, resulting in the fallback of 2.

🇩🇪Germany Grevil

All done, please review!

🇩🇪Germany Grevil

LGTM!

🇩🇪Germany Grevil

The current release broke compatibility with Drupal 10! A quick fix is provided here 🐛 Optimize the getMentionsFromEntity() method Active . I'll create a new release with the fix ASAP, apologies for the regression!.

🇩🇪Germany Grevil

Thank you for taking care of it @kksandr!

🇩🇪Germany Grevil

grevil created an issue.

🇩🇪Germany Grevil

grevil created an issue.

🇩🇪Germany Grevil

Alright, all done, please review finally @anybody!

🇩🇪Germany Grevil

Adjusted accordingly! Merging!

🇩🇪Germany Grevil

Yea I guess so, people already created critical issues for this...

🇩🇪Germany Grevil

Can not reproduce this issue. Have you tried flushing all caches after update using `drush cr`?
What Drupal Version are you using?

🇩🇪Germany Grevil

grevil created an issue.

🇩🇪Germany Grevil

All fixed, please review again!

🇩🇪Germany Grevil

@anybody! Nono, this is a success! The tests now actually show failures! Before it simply timed out after 1 hour.

🇩🇪Germany Grevil

Thanks once again @ressa!

LGTM!

🇩🇪Germany Grevil

Definitely makes sense!

Tests are currently failing. Also, this requires further tests, thanks!

🇩🇪Germany Grevil

grevil made their first commit to this issue’s fork.

🇩🇪Germany Grevil

grevil made their first commit to this issue’s fork.

🇩🇪Germany Grevil

Thanks, @ressa!

I added the info to the module page. I'll commit the README change, but we should POSTPONE this issue, until it is fixed in core!

Production build 0.71.5 2024