Netherlands
Account created on 8 December 2015, over 8 years ago
  • Technical director at SWISΒ 
#

Merge Requests

More

Recent comments

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Oh wow, good call. I think we should actually be able to do that.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

bbrala β†’ made their first commit to this issue’s fork.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

bbrala β†’ made their first commit to this issue’s fork.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

bbrala β†’ made their first commit to this issue’s fork.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

All green! :D

Did drop d9 support because of issue with traioling commas. But meh.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

bbrala β†’ created an issue.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

bbrala β†’ made their first commit to this issue’s fork.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Closing and removing bot tag/

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Closing and removing bot tag/

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

All green, added gitlab also.

Keeping active for new issues from the bot if needed

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

bbrala β†’ made their first commit to this issue’s fork.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands
πŸ‡³πŸ‡±Netherlands bbrala Netherlands
πŸ‡³πŸ‡±Netherlands bbrala Netherlands

bbrala β†’ created an issue.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

All green :D

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

All green, think we are fine :)

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

bbrala β†’ created an issue.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

bbrala β†’ created an issue.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Thank you. But will not be merging this, we do not use comments in this way. I'll put in gitlab and phpcs in a d11 issue.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Sorry, but already working on this :)

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

bbrala β†’ created an issue.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

All good, agreed :) Moving to proper queue.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Also updated the 3.x issue with the same changes, then upgrading wont be too hard for users: πŸ“Œ Automated Drupal 11 compatibility fixes for paragraphs_edit Needs review

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

bbrala β†’ changed the visibility of the branch 3460956-d11-compatibility to hidden.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

bbrala β†’ made their first commit to this issue’s fork.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

This seems fine, perhaps we should merge ✨ Drupal 11 compatibility and gitlab Needs review first and get that into 3.0 to get gitlab running also for phpstan/cs validation.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Tryinig to get through all issues.

Should the permission migration be different? Wouldnt the user need like content edit permsissions, the authenticated role seems a little optimistic, this could also be a user that can just login to comment or something?

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

I know it is failing, but that is next major since paragraphs is not yet compatible with d11. Declaring compatibility is fine since we this modules is not holding back d11.

Added gitlab yaml with phpstan which will help with developement, did do a few fixes for codestle :)

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

bbrala β†’ created an issue.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Make opt-in settings copy-pastable.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

I think this is fixed now. Problem was indeed the difference of `include=` versus no include paramter at all. I would love a review on this, perhaps test this patch on your site.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Tried to reproduce in tests, couldn't reproduce the first case (IS) but seems i can reproduce the second case by @cb_dewr.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

We will postpone this until 11.1.x branch is openend.

Will note the needs review, and will get to this when 11 is released :)

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Ignore me, missed a few comments o_O

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Do we want to drop node 20 support? Cause that is what we are doing with glob.

https://github.com/isaacs/node-glob/compare/v10.4.4...v11.0.0

There is also https://github.com/isaacs/node-glob/releases/tag/v10.4.5

But ths commit "whatever, just allow any engines" doesnt make very happy :x

https://github.com/isaacs/node-glob/compare/v10.4.4...v10.4.5

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Changes to revert the rename as per @alexpott's questions in slack are all good. Old name does not remain. Waiting for green pipeline, but code looks good.l

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Meeting calendar is posted on the project page?

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Hmm, a 404 is a little weird on the relationships, i agree. 422 would indeed make more sense.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Unfortunately: JSON:API requires use of the JSON:API media type (application/vnd.api+json) for exchanging data.

So it would be off-spec to use something else. You could discuss alternative options, like go offspec yourself by changing the header with some custom code. But we cannot change how core does this.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Yeah, report it, but then it wont hold back upgrades since the chance for it to actually be an issue is minimal.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Oops missed delte indeed. Same set.

Silly if I mistyped the strpos, will fix.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

There were appearantly a lot of api.php files which had php errors inthem, breaking phpstan. Thats why it was moved to bleeding edge and not enabled by default. If we can split those we could enable it by default in core since those will not break.

I could to verify try to do a run with update bot with the api.php enabled for all modules, see what happens.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

To be honest the changes itself look good, all sane changes. Nice to see the sniffing out of the weird mocking going on. PhpUnit next major indeed fails on RDF, and we can ignore that for now.

All linting is failing (cspell, phpcs and phpstan). I do feel we need to start fixing those, but that should be another issue.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

In the end we need a source of truth. I'd opt for gitlab for that eventually I think. But a maintainers.txt is also usefull for history.

Thanks for adding the related issue, that is usefull.

At some point we should also add codeowners possibly. For the different parts of Drupal I think. But that one for later, and very out of sco0e of this issue.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Ok, so what i understand.

/jsonapi/node/page?include=: A page with includes cleared.

is cached the same as

/jsonapi/node/page: A page with default includes.

That indeed seems like a regression. We should able to test that pretty easily in this test somewhere:

JsonApiDefaultsFunctionalTest

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Resolved all feedback.

  1. Add an admin user and check for an admin role in the testing
  2. Improve some comments to help developers use this feature.
  3. Update tests.

All good I think :)

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

I would assume no tests should need to change. But it will require diving into the issue references to see what we are looking for. Can't currently give a real complete anwser.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Removing novice tag since it is not as simple as initially hoped.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Thanks so much guys for working on this. I've tested the changes locally on 9, 10 and 11. Also tested on a running public api which works perfectly.

Al seems to work as intended. Lovely. Merging and releasing a RC for this

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Yes, i think this is fine like this since it is only as docblock.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Keeping Drupal 9 support is kinda annoying, not sure how you see that with the return type changes. Those changes were introduced in 10. I could conditionally load imposters, but that feels kinda crap.

Not sure how you see this.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Some small questions for now.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

General consensus is not to do this, we have had another month of time to see other opinions. Will close this as won't fix.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

As discussed in the coding standard meeting of 22th of may, since there is not more activity we are closing this for now.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

This issue was discussed in the coding standard meeting of 21st of May by @larowlan, @longwave and @quietone

Since nowadays we use Classes as references to events this is outdated.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

0️⃣ Who is here today? Comment in the thread to introduce yourself. We’ll keep the meeting open for 24 hours to allow for all time zones.

1️⃣ What topics do you want to discuss? Post in this thread and we’ll open threads for them as appropriate.

2️⃣ Action items

2️⃣.1️⃣ Approve minutes for previous meeting(s)

4️⃣ .1️⃣ #3339746: Coding style for PHP Enumerations β†’

4️⃣.2️⃣ #3324368: Update CSS coding standards to include PostCSS and Drupal 10 β†’

6️⃣ Old issues (edited) 

6️⃣.1️⃣ #1991932: Agree on a standard for assigning references to variables β†’

6️⃣.2️⃣ #1561464: Introduce @was phpDoc directive to link/reference to previous/renamed functions β†’

6️⃣.3️⃣ #1567920: Naming standard for abstract/base classes β†’

6️⃣.4️⃣ #1539738: Define and document a policy on list() syntax β†’

6️⃣.5️⃣ https://www.drupal.org/project/coding_standards/issues/782016 β†’

7️⃣ Issues in review blog post (edited) 

6️⃣.7️⃣ via @mstrelan #2918440: Define a standard for documenting data providers in PHPUnit-based tests β†’

Participants:

mstrelan, quietone, larowlan, bbrala, urvashi_vora, catch, apaderno, borisson_

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

This meeting was not run.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

bbrala β†’ created an issue.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

bbrala β†’ created an issue.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

bbrala β†’ created an issue.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

bbrala β†’ created an issue.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

bbrala β†’ created an issue.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Gitlab tells us it will merge without a problem.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Hopefully that is possible. It would need to simulatie a core module and deprecation

Production build 0.69.0 2024