Norway
Account created on 3 November 2010, over 14 years ago
#

Merge Requests

More

Recent comments

🇳🇴Norway eiriksm Norway

eiriksm created an issue.

🇳🇴Norway eiriksm Norway

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

🇳🇴Norway eiriksm Norway

Great! thanks! ♥️🚀👌

🇳🇴Norway eiriksm Norway

Released 2.0.2 which should be compatible just now. Thanks!

🇳🇴Norway eiriksm Norway

In this MR I also added config schema (this is in theory required to run tests out of the box).

However, when adding a schema for the test mode, the error no longer happens in the tests. So I pushed a commit where I temporarily disabled the strict schema check, and demonstrated that the tests will catch the bug: https://git.drupalcode.org/issue/vipps_recurring_payments-3509640/-/jobs...

Later it was re-enabled, and the latest test run includes both the schema fix, added back strict schema for tests, and the actual fix for the config class

🇳🇴Norway eiriksm Norway

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

🇳🇴Norway eiriksm Norway

Sounds like a great idea, then!

Merge requests welcome!

🇳🇴Norway eiriksm Norway

What you committed is more or less exactly what I have been implementing only in custom code. So this is actually perfect!

Thanks!

🇳🇴Norway eiriksm Norway

Oh my concerns are more towards usability and confusion.

If I have a colleague, for whom it will be faster to click a couple buttons on stage or prod and then get a PR out of it. Great. They can add a token to their user

The administrator of my client website however. They will see the words access token and think this looks like an error or is noisy. They don't have a GitHub user. Heck, they don't know what GitHub is

So it's rather easy for me to fix this in custom code, but I wanted to hear if this was a scenario the actual contrib project wanted to cater for directly 🤓

🇳🇴Norway eiriksm Norway

Created https://www.drupal.org/project/linkchecker/issues/3506219 📌 Create tests for showing the translated entity link Active for the tests

🇳🇴Norway eiriksm Norway

Alright looks simple enough. Let's do tests in a follow up

Thank to everyone who worked on this! ♥️🚀

🇳🇴Norway eiriksm Norway

Could someone provide exact steps to reproduce from a clean install of Drupal in the summary please? 🤓✌️

🇳🇴Norway eiriksm Norway

Oh there are steps here now, awesome!

That should make it trivial to write a test then!

🇳🇴Norway eiriksm Norway

There are still no clear steps to reproduce this issue, and still no tests 🤓✌️

🇳🇴Norway eiriksm Norway

I will not merge this since d9 is legacy, but leaving it here for being able to patch if necessary

🇳🇴Norway eiriksm Norway

That being said, it's still possible to trigger in other ways than with a constant. It was just randomly how I discovered it back in the day (can't even remember reporting it hehe time flies)

🇳🇴Norway eiriksm Norway

eiriksm created an issue.

🇳🇴Norway eiriksm Norway

eiriksm created an issue.

🇳🇴Norway eiriksm Norway

eiriksm created an issue.

🇳🇴Norway eiriksm Norway

eiriksm created an issue.

🇳🇴Norway eiriksm Norway

Released 2.0.4: https://www.drupal.org/project/twigjs/releases/2.0.4

Thanks to everyone who worked on it in this issue as well. The 8.8 part was never included but 8 is totally unsupported at this point so no point in tweaking that 🤓

🇳🇴Norway eiriksm Norway

eiriksm created an issue.

🇳🇴Norway eiriksm Norway

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

🇳🇴Norway eiriksm Norway

This looks good to me.

The only thing is we are per the info file still compatible with Drupal 9.x here. Is it allowed to pass them in that order on Drupal 9? Just asking since I don't have it in front of me, and I think it is?

🇳🇴Norway eiriksm Norway

Probably a wording from the Drupal 6 or 7 version lingering around.

Do you have a suggestion on more accurate wording? 🤓

🇳🇴Norway eiriksm Norway

I can not approve the MR in gitlab, but I reviewed it nevertheless :)

🇳🇴Norway eiriksm Norway

Still needs tests ✌️🤓

Also, please use a merge request so the testing pipelines can run

🇳🇴Norway eiriksm Norway

Woah that reads as very angry or strict

Sorry about that.

I think you make good points and the last comment is only answering your question. I guess it can still be listed on the module page of linkchecker though, I'll see what I can do

🇳🇴Norway eiriksm Norway

1. It's listed if you click the link "projects that extend this"
2. No. My personal opinion is that submodules are an anti pattern. For different reasons, but limiting it to the following

- Separate modules make it possible to have asynchronous releases. In most cases what you want, really
- Separate modules come from (and encourage) community effort. Coincidentally I am also a maintainer of linkchecker but that was not the case when I contributes this module back in the day. This fostering of development is positive in my opinion

🇳🇴Norway eiriksm Norway

I'm setting back to "needs review" since there were some changes done after the last review step

🇳🇴Norway eiriksm Norway

Amazing, thanks ♥️

🇳🇴Norway eiriksm Norway

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

🇳🇴Norway eiriksm Norway

Hm, you may be right about this. I guess I was not up to date on the role flag, I thought it was possible to override the permissions even if the role was admin.

Happy to review some code suggestions if you have them :)

🇳🇴Norway eiriksm Norway

We need a MR for the tests to run, and for it to be super easy to merge :)

🇳🇴Norway eiriksm Norway

Just to also present a solution.

I Just went with logging out by myself with what I know works on my project right here:

    $this->drupalGet(Url::fromRoute('user.logout.confirm'));
    $this->getSession()->getPage()->find('css', '#user-logout-confirm [name="op"]')->click();
    $this->drupalResetSession();
🇳🇴Norway eiriksm Norway

Awesome, thanks 🚀🎉

🇳🇴Norway eiriksm Norway

Thanks for picking this up so much later than the original post ♥️🤓

The strings still need to be translatable though, so back to needs work for that

🇳🇴Norway eiriksm Norway

Hm, that seems counterintuitive with the name of the module :)

Also, it's quite a different attack surface to edit a super user, compared to an admin role, which may or may not be super users per se.

Personally I would rather make this module do nothing if that setting is enabled. Thoughts?

🇳🇴Norway eiriksm Norway

Cherry picked to 2.0.x and 2.1.x

🇳🇴Norway eiriksm Norway

Obviously I'm not an expert, but to see two crucial, but in no way related modules being removed with commerce_oci_checkout is a bit scary to me.

Right. That's a strange feature of composer, but what you are seeing is actually metapackages. The actual module commerce is still installed.

Did you try the patch mentioned? 🐛 Wrong dependency injection of "session.attribute_bag" Active that is

🇳🇴Norway eiriksm Norway

Thanks! ❤️

Could we get that in a merge request to get the tests running please? 🤓️

Production build 0.71.5 2024