🇳🇱Netherlands @bbrala

Opened a MR against 11.x with the patch.

Add parent issue, since me and casey think we can solve this globally.

Adding credits to people who investigated this, casey as colleague and people from 🐛 ResourceObjectNormalizer::getNormalization can result in max-age drift when different sets of fields are requested Active .

bbrala → created an issue.

Added code to extract applies rectors. If we can get reflection happening the min version could be determined based on the class, make an issue in rector to see if we can get that. See: rectorphp/rector/issues/8644

This has been its email for ages i think, since the bot has been around since d9 :) So this is fine really.

Same for shortcode:

<file name="modules/contrib/shortcode/src/Plugin/ShortcodeBase.php"><error line="403" message="Call to deprecated method renderPlain() of class Drupal\Core\Render\RendererInterface. Deprecated in drupal:10.3.0 and is removed from drupal:12.0.0. Use Drupal\Core\Render\RendererInterface::renderInIsolation() instead." severity="info"/></file>

Other deprecation though.

For rest views in latest i do see an unfixable error:

file name="modules/contrib/rest_views/rest_views.module"><error line="85" message="Call to deprecated method getName() of class Drupal\Core\Extension\ModuleHandlerInterface. Deprecated in drupal:10.3.0 and is removed from drupal:12.0.0. Use Drupal::service('extension.list.module')-&gt;getName($module) instead." severity="info"/></file>

We don't have rector support for that.

We have ran into this issue, and my colleague @casey has found this problem happening in other parts of the caching system also. We ran into this because we use max-age in combination with a time for anonimous visitors to have access to a private file and we noticed that cache was not invalidated a tthe right time. This was also a combination of different caches that stack their max-age and therefor ending up with caches that don't invalidate at the right time. This gets even worse, when certain caches invalidate or get purged because of cache size this can actually result in issues around an invalid state which results in WSOD's. We've seen this in cron and certain very big sites.

As we talked about this this morning, we think we might need a META issue around this, where max-age should consider expires and be recalculated if a later cache uses earlier caches. This would probably fix a lot of issues.

I hope i can make a good writeup soon so we can work on this. We can probably fix this in steps though and start calculating max-age based on bubbled max-age and expires.

Caching is hard lol

adding some related issues also

After way too much delibertaion, we decided to make @AstonVictor comaintainer for paragraphs_edit. Welcome, and thank you :)

Hehe, thanks Alex :)

Congrats Kim! :)

You still need to chnage the target of the mr I think. It's still targeting 10.4

Sounds like a playing. I'll wait for Kim to update and review the changes.

bbrala → created an issue.

Although I agree with you, i'm weary to do this in the current cycle because of the mass renaming that will need to happen. It would also mean new mr's etc etc. :(

I promise to improve this next cycle.

Ok, all is green and generating proper info.

Test did fail, guess review bot didnt find this one yet ;p

bbrala → created an issue.

Thanks catch. And maybe it is ;)

The JSON:API test just means it needs a rebase i think, its the test that failed in other mr's also.

Assuming tests don't fail this is exactly what we need to improve project_analysis.

THis is all done. Followup is posted, cr also.

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)

#3440953: Coding Standards Meeting Tuesday 2024-04-23 2100 UTC →

2️⃣.2️⃣ TBD

3️⃣ Fixed since last meeting

4️⃣ RTBC issues

4️⃣.1️⃣  #3295249: Allow multi-line function declarations →  (edited) 

4️⃣.2️⃣ #3339746: Coding style for PHP Enumerations →

4️⃣.3️⃣ #3074131: Use null coalescing operator ?? instead of a ternary operator with an isset() condition →  (edited) 

4️⃣.4️⃣  #3324368: Update CSS coding standards to include PostCSS and Drupal 10 →

5️⃣ New Issues

5️⃣.1️⃣ #3444003: Change sniff rule for @todo to be capitalized and sniff non-php files →

5️⃣.2️⃣ TBD

6️⃣ Old Issues

6️⃣.1️⃣ #3117816: PHP 7 Group Import Declarations →

6️⃣.2️⃣ #2304937: Please remove this requirement: 'WARNING | Interface names should always have the suffix "Interface"' →

7️⃣ Wrap up. Keep chatting in the threads and and news one. Meeting is open for 24 hours.

Participants:

urvashi_vora, catch, longwave, bbrala, Kingdutch, larowlan, quietone, borisson_, acbramley

Everything in #11 has been adressed.

Think this is fine. BC is a bit hard to fully consider, but i think we have enough. All threads have been resolved, RTBC for me <3

I went through the MR and have some comments. All and all it is a good implementation of what we talked about (quite) a while back.

Change record is available.
Issue summary seems up to date, although format is a little more freeform, would prefer to move to default setup. So keeping that tag.

Hi! We've talked this through, we would love to have you as a maintainer :)

Could we perhaps talk on drupal slack? (bbrala) to sync on a few details like BC, releasemanagement, credits etc?

Awesome to see this moving!

Quick contrib search:

1. jsonapi/Controller/FileUpload - 0 usage
2. jsonapi\Controller\TemporaryJsonapiFileFieldUploader - 0 usage
3. jsonapi.file_upload - 1 usage

Seems we are not really doing anything impactfull against contrib.

I've checked the CR, its small, but fine i think.

BC paths make sense for new arguments for the controller.

Gone though the code, have some questions :) This feels rather close, and i'll be quite happy to have the "Temporary" class killed :D

We talked about this in the codingstandards meeting of the 8th of may. Concencus was not to allow this as per reasons above.

https://drupal.slack.com/archives/C02LJCF78E8/p1715159892900999

This was talked about in the coding standard meeting of 8th of may. Concensus was to not do this.

Multiple reasons;

1. Clear in file paths what it is
2. We cannot scope by directory, way to late for some sort of contracts setup to do this
3. We are kneedeep into the transition, so we cant easily move

As such we are closing this issue. We kinda feel sorry for the fact this stayed open for so long, since the time to discuss this has far gone now.

https://drupal.slack.com/archives/C02LJCF78E8/p1715159914515049

Updated new code to standard again and fixed a conflict with 11.x

All feedback has been resolved. Think this is ready, ty for the work here.

Might need a rebase, but that single test is failing consistently.

Extra note, also read through the cr's

Found a way (non existing node)

bbrala → created an issue.

Ok, that breaks the issue summary because of d.o. replacement... o_O

Not sure how to communicated that then.

For option #8 we might need a # in front to supprt issue linking in gitlab. See: https://docs.gitlab.com/ee/user/project/issues/crosslinking_issues.html

And for the service. Think we are golden, no bc path.

https://git.drupalcode.org/search?group_id=2&repository_ref=11.x&scope=b...

I think we are ok with no bc path for the topdocumentnornalizer.

https://git.drupalcode.org/search?group_id=2&repository_ref=11.x&scope=b...

No usage. Just to be sure I'll also check the service

Went through the changes again. I have some feedback of thinigs that have changed in core since. Also i'm afraid we need a BC path for the contructor addition. I think @larowlan just missed that.

bbrala → changed the visibility of the branch 3446107-use-null-coalescing-10.4 to hidden.

Made a fresh Mr against 10.4.x seems that one is fine.

Think something went wrong, spellcheck failed. Did rebase go well?

Changes look good, you also changes the 2 liner, great :)

Wish i was at a table in portland :(

Cool, I'll make a new branch for 10

Added a change record to keep it moving :)

[#3446111]

Published the CR, since all is done i'm going ahead and set this to fixed :) Thanks everyone.

Added the rule, there are some violations on core since the original big push to change this. Fixed those autmaticly.

Think you misreferenced @quietone. I didnt find one specifically for this change. So made 📌 Enforce null coalescing operator ?? instead of a ternary operator through phpcs Fixed .

bbrala → created an issue.

Fixed credits

I would not support this.

1. Less readable
2. Worse Diffs
3. Rather unknown syntax.

quietone → credited bbrala → .

Ty @fjgarin, safe travels!

Have we done a run with the extra modules specified? Don't see that. Seems important.

Awesome, this will help a lot while developing jsonapi. :)

Ok interesting, fair enough. I'll put in on my list and will probably start to see if i can create some failing tests.

I would probably template the script also. I know that one issue a module might have is that it wants to check submodules, which would mean enable more modules than the main module.

For example: jsonapi_extras_defaults.

Adjusting the flow is a lot easier if the base script steps are templates l. Like: install and chrck or something.

Other option is a var that does extra_enabled_modules or something.

Hmm, interesting bug. Was just talking about something similar with a colleague earlier this week.

Not sure is this is truly jsonapi or more caching. I'll come back to this issue soon.

I'm gonna be bold and keep rtbc. Seems like a really really minor change.

Hmmz, I think this needs a last minor change.

$errors[] = $error['title'] . ': ' . $error['detail'];

Should be:

$errors[] = $error['title'] . '(' . $error['status'] . '): ' . $error['detail'];

That will help in some cases.

Update 10.3 mr also, all done <3

11.x is updated, ill push the same changes to the 10.3 mr (excluding the BC removal)

Gone through all responses and think a few more can be closed @Wim Leers :) Thanks for the awsers.

Note: Wim asking for feedback from someone with config experience (or maintainer there). Quoting for visibility:

https://git.drupalcode.org/project/drupal/-/merge_requests/6704#note_305097

    // @todo this is inaccurate — it won't work for e.g. `field.field.*.*.*`. We
    //   need the inverse of \Drupal\Core\Config\ConfigManager::getEntityTypeIdByName()
    //   To build that, we would need to expand the `@ConfigEntityType`
    //   annotation with a "id_parts" key-value pair, which defaults to 1, and
    //   FieldConfig, FieldStorageConfig, EntityViewMode etc. would specify.
    $config_schema_type_name = $entity_type->getConfigPrefix() . '.*';
    $config_schema_type_definition = \Drupal::service('config.typed')->getDefinition($config_schema_type_name);

Did a quick fix for the logOut issue bases on the changes here: 🐛 User logout is vulnerable to CSRF Needs work . There where changes in how we logout, we need those changes in this test also.

I tried removing the mink session reset, but that was needed, so put it back in.

I've fixed the pipeline by using drupalResetSession in the test, which logs out the user through code.

It is kinda weird though, that loggin out didn't work through the interface, but that is isolated from this issue.

Moved interface up, and added a change record → describing the improvement.

There was an issue with user login which broken 10.3 head. Which after also has a few other things fail. It might be something there. Too tired to check today though.

Did the dance, unfortunately a true rebase onto the other branch did not work, so force pushed the diff. Setting to NR, pipeline is pending.

Composer.loxk I'm not sure about, but this mr does not add it so let's keep it.

Don't see issues adding this, quite helpfull in Europe.

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

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

bbrala → created an issue.

Screwed up a rebase lol ;p

Anyways, all should be fine again.

Huh, there was a second one? Might be mixing up two issues though. I'll fix it

agentrickard → credited bbrala → .

agentrickard → credited bbrala → .

agentrickard → credited bbrala → .

Bot is lying. It applies.

Rebased and fixed 2 remaining (new) issues in 11.x

The core issue has been fixed. Mr for 10.3 should pass again.

https://www.drupal.org/project/drupal/issues/3443248 🐛 Can't log out on translated site in tests - causes issues for 10.3 umami test Fixed

Yay, rerunning some mrs :p