🇳🇱Netherlands @bbrala

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

Love the remove in Drupal 9 comment there hehe.

Don't see how this would break anything removal is clean. Lets fix this leftover.

Really only see the isnew issue. The response to larowlans comment seems valid, and personally I agree with mrx on that.

If we can update that I'll rtbc again.

I recently discussed thie with catch.

Using the header and vary on that is very dangerous for cache purposes. It will make very very many variations, which results in bad caching and performance. Not to mention possible memroy issues in caches or high cache trashing.

We did think about how we could possible work around that.

What if;

We normalize the prermutation of the language into the resulting language, and cache that normalization and the resulting language.

So perhaps a site with en_EN and fr_FR. A browser comes along and tells the site "Hi i'm en-GB,en-US;q=0.9,en;q=0.8". The site would find out what this will end up as so

en-GB,en-US;q=0.9,en;q=0.8 magic => en_EN, we cache this and threat serve cache asif en_EN. This would mean (at least on the drupal side) only the records for inLang => outLang exist, which are pretty small and we dont end up with loads of difference permutations.

Not entirely sure how we would implement that, but feels like a way to have drupal cache be happy.

The little devil on my shoulder also has a voice though. Which does tell me this is kinda a. can of worms as this could also easily break caching on the caches in front of Drupal, be it varnisch or maybe smoething like cloudflare. That is kinda the scary part. Perhaps contrib is better to have this live, instead of trying to get this into core.

Fixed phpstan issues and rebased (me like cache).

I merged main into the branch, still, i didn't see any merge conflicts, so it might have been just a hickup by gitlab (which is not uncommon).

longwave → credited bbrala → .

Hmm, unfortunate. I'll have a look tonight to fix the conflict.

smustgrave → credited bbrala → .

jpvos → credited bbrala → .

jpvos → credited bbrala → .

jpvos → credited bbrala → .

Pro tip, print the list in comment 57 and hang it on your monitor ;) Saves a lot of time while getting used to it.

Yeah that should be a new issue.

Talked with catch avout a possible angle of attack which would make this viable (possobel) from a variation and perdormance perspective. Will post that sometime soon.

Almost got scared at this one, went through all function, and the count did add up, nothing missed.

One thing that has been missed though is perhaps some tests:

Drupal/FunctionalTests/Install/ has multiple tests that actually write an install function to file. Shouldn't those also be changed to add the return?

Example:

file_put_contents("$path/my_distribution.install", "<?php function my_distribution_install() {\Drupal::entityTypeManager()->getStorage('path_alias')->create(['path' => '/user/1', 'alias' => '/root-user'])->save();}");

Another lveoly clearnup. Starting to repeat myself, but changes look good. When lookin in code, this is exactly what should have been changes. See no missed code. Count didnt add up initially, but that is because of the string in "update_test_last_removed.into.yml".

Changes makes sense, had a few extra hits wehen looking around for missed ones. But those hits were in

system.install

Seems to cover all bases :) Nice one.

Checked local code, found 47 hits that need changing, code has 44. But that makes sense, becaulse of:


function twig_theme
function _drupal_maintenance_theme
function drupal_maintenance_theme


All good :)

Rebased, added experimental flag, went through threads and resolved what was needed.

bbrala → created an issue.

This is not really something that can be installed through this project though... Is a logo needed then?

Usefull list of types with description.

https://github.com/pvdlg/conventional-changelog-metahub#commit-types

Last comment was fixed. So as per #15, this is RTBC for me. Makes quite a few things a bit more consistent. Although this test class does hurt my eyes.

This ready for review?

gábor hojtsy → credited bbrala → .

I'm so confused. I tried that earlier. But yay!

gábor hojtsy → credited bbrala → .

Yeah alt texts would be good to add. Think we should be all ready then

bbrala → created an issue.

bbrala → created an issue.

Left a small question.

Went through the whole thing and changes are looking good.

It does make me dream of a less magic way of testing jsonapi again. But that mountain is kinda daunting.

This issue is pretty closed. Could you an new issue about this so we can investigate?

I think * is protected right? So all branches should be protected if understand correctly.

Hmm, yeah guess so. Seems the page is a little wider that subsystem maintianer. Also this could also be used for topic maintainer. So I agree maintainer is a little better.

Thanks! I've updated the CR a little.

I prefer to test this, ive put some time into this to see if i could test it with a kernel test. Seems i can use the resolver properly, just need to do the second level of node and see if i can reproduce this. I keep getting an itch this needs testing.

Very rough work in progress.

class JsonApiDocumentTopLevelNormalizerTest extends JsonapiKernelTestBase {

  use ImageFieldCreationTrait;
  use ContentModerationTestTrait;
  use UserCreationTrait;

  public function testRelationshipVersionIncludes(){

    $this->assertTrue($this->container->get('module_installer')->install(['content_moderation'], TRUE), 'Installed modules.');


    $workflow = $this->createEditorialWorkflow();
    $this->addEntityTypeAndBundleToWorkflow($workflow, 'node', 'article');
    $this->type->getEntityType()->setHandlerClass('moderation', ModerationHandler::class);


    Role::create([
      'id' => RoleInterface::AUTHENTICATED_ID,
      'permissions' => [
        'access content',
        'view any unpublished content',
        'use editorial transition publish',
      ],
      'label' => 'Anonymous',
    ])->save();

    $this->setCurrentUser($this->user);

    $this->includeResolver = $this->container->get('jsonapi.include_resolver');
    $this->resourceTypeRepository = $this->container->get('jsonapi.resource_type.repository');

    $this->createEntityReferenceField(
      'node',
      'article',
      'field_entity_reference',
      'Related entity',
      'node',
      'default',
      ['target_bundles' => []],
      1
    );

    $node = Node::create([
      'title' => 'original node',
      'type' => 'article',
      'uid' => $this->user,
      'body' => [
        'format' => 'plain_text',
        'value' => $this->randomString(),
      ],
      'field_tags' => [
        ['target_id' => $this->term1->id()],
        ['target_id' => $this->term2->id()],
      ],
      'field_image' => [
        [
          'target_id' => $this->file->id(),
          'alt' => 'test alt',
          'title' => 'test title',
          'width' => 10,
          'height' => 11,
        ],
      ],
    ]);

    $nodeWithReference = Node::create([
      'title' => 'original node with reference',
      'type' => 'article',
      'uid' => $this->user,
      'body' => [
        'format' => 'plain_text',
        'value' => $this->randomString(),
      ],
      'field_tags' => [
        ['target_id' => $this->term1->id()],
        ['target_id' => $this->term2->id()],
      ],
      'field_image' => [
        [
          'target_id' => $this->file->id(),
          'alt' => 'test alt',
          'title' => 'test title',
          'width' => 10,
          'height' => 11,
        ],
      ],
      'field_entity_reference' => [
        ['target_id' => $node->id(), 'entity' => $node],
      ],
    ]);
    $nodeWithReference->save();

    $node->set('title', 'updated node');
    $node->setNewRevision(TRUE);
    $node->set('moderation_state', 'draft');
    $node->save();

    $nodeWithReference->set('title', 'updated node with reference');
    $nodeWithReference->setNewRevision();
    $nodeWithReference->set('moderation_state', 'draft');
    $nodeWithReference->save();

    $parentVid = $nodeWithReference->get('vid')->value;
    $childVid = $node->get('vid')->value;

    $resource_type = $this->container->get('jsonapi.resource_type.repository')->get('node', 'article');
    $resource_object = ResourceObject::createFromEntity($resource_type, $nodeWithReference);
    $includes = $this->includeResolver->resolve($resource_object, 'field_entity_reference');

    $jsonapi_doc_object = $this
      ->getNormalizer()
      ->normalize(
        new JsonApiDocumentTopLevel(new ResourceObjectData([$resource_object], 1), $includes, new LinkCollection([])),
        'api_json',
        [
          'resource_type' => $resource_type,
          'account' => NULL,
          'include' => [
            'field_entity_reference',
          ],
        ]
      );
    $normalized = $jsonapi_doc_object->getNormalization();



    // Todo:

    // Node 1  (static) -> reference node 2 -> reference node 3
    // Update node 3 to have a working copy.
    // Ask for working copy. Might need

  }
}

;(

Darn. Did not check after doing the whole thing in the editor.

If noonen does I'll Retro tomorrow. Did copy paste.

Awesome!

I'll redo it.

@smustgrave can you force push again?

I just noticed also, my 11.x was polluted by a small mini commit.

Fun fact.

Issues now show the merge button in the D.O. interface. I dont dare to try it...

Back to NR after rebase.

Note; when you have done a composer install in core directory, things will fail ;p

And lol steven ;x

As per #26

Rebased and rebaselined. Will probably need to do again before commit, so here's the steps I took.

git reset --hard 96204beed3f
git rebase 11.x
composer install
vendor/bin/phpstan analyze --configuration=./core/phpstan.neon.dist --memory-limit=-1 --generate-baseline=core/.phpstan-baseline.php
git add core/.phpstan-baseline.php
git commit -m "Update baseline"
git push --force

Thanks longwave, marking as fixed.

I've uploaded all the images and made sure the structure (with step a and b) is actually readable.

I'd also add links, but I cannot since the settings pages are not available to my role. I think this is pretty good, although it good be better, I'd still say docs are updates.

One thing we might need to do is make a blogpost regarding this change, and perhaps have a better place to ask for retargeting and such. If we for example have a channel to ask for retargeting of issues we could check there and update where needed, streamlining the process.

Upload all the images

This one needs work since mettings have not been parsed yet.

Closing, all seems good, thanks! Adding quietone for review.

Closing, all seems good as mentioned earlier. No corrections since.

Hopefully another vacation time meeting. Seems all complete and ready to go.

Very much vacation time it seems, all seems included.

I wonder if core comitters have to do something in the interface to allow all maintainers to edit or something. There are some settings around allowing edits, might be something that has a secure default.

Hehe no worries drumm. :) New territory here

So seems to work as it should yay!

Next up is getting the docs into d.o.

The source document is here so we can copy paste

https://docs.google.com/document/d/1OcYqPrtkQDlFXQs6-xSbcCsEK85In5JSxEUW...

Step 3: tried resolving a random thread (and unresolving) which worked in this mr https://git.drupalcode.org/project/drupal/-/merge_requests/1036#note_40479

Step 2: tried pushing to 11.x and got a permission denied.

Step 1: tried retargeted a random MR successfully - https://git.drupalcode.org/project/drupal/-/merge_requests/5271

That is awesome! Thank you! I will make some time, hopefully later today to have a look to this and see if this works as intended.

Hehe yeah, let's make exceptional caching into super exceptional caching haha

🐛 ResourceObjectNormalizer::getNormalization can result in max-age drift when different sets of fields are requested Active has landed :D

I think 📌 Make language switcher block cacheable Postponed could be handled without evaluating the render cache i think. Hoepfully we can get some activity there.

For this issue, we should try and keep the blockers as small as possible, seems it is a pretty short list.

This is blocked on 🌱 Optimize render caching Needs work curretnly. But im not sure thats needed. Perhaps we can find another solution that allows us to fix this issue. That would unblock #2352009: [pp-3] Bubbling of elements' max-age to the page's headers and the page cache → more, and i feel like the whole render cache issue is a bit to big and complicated.

Tried to make the comment a bit more understandable. It is still hard imo, but more clean in many ways :)

RTBC since its only a small comment change.

I was also thinking of using composer autoload to override a class to fix this, but that would mean overwriting a big class like CoreExtension, or smaller TwigFilter, but that does kinda feal like using a hammer to screw in a screw. We could though override TwigFilter, its quite a small class, but that adds complexity when updating at some point.

The solution with the drupal_spaceless would work, the issue is that the deprecation message is triggered in the ExpressionParser. This is really early in the process, and i have not found a way to really hook into that early enough.

We might be able to do some 'hackery' to adjust the specific filter in CoreExtention to not have deprecated. But i've not found a way for that.

Fair point. Will do

bramdriesen → credited bbrala → .

Rebased and small tweak to test method order and comment. All ready.

CR drafted, child issue for #6 created in 📌 aInvestigate if ComponentValidator should have soft dependency on justinrainbow/json-schema Active

bbrala → created an issue.

The intial setup from the PDF needs to be done, make the branches and tags protected (wildcards) and give maintainers the permission to allow to create.

This needs to be done by someone who has those permissions in the Drupal project. I'm more than willing to create the guide then for adding and managing the subsytem maintainers.

We are all agreed this is a good idea, so all is fine, and as soon as we fix this we can start helping out with retargeting branches etc, taking off load from the committers.

(unhappy i forgot this issue during barcelona, would've just pulled someone to do it :P)

Isnt the issue that there is extra newlines before/after/between tags, not nessesarily in the comments. The comment could still be formatted, we just need the output not to use newlines around them? We could even apply a drupal_spaceless filter on that specific node when it is visited by our nodevisitor and have that output be ok? But then again, we might as well replace apply spaceless with apply drupal_spaceless and start helping contrib move along.

Still though, that would not solve the fact it is deprecated and the fun contrib will have with spaceless being gone. Been looking at replacing it with out own, but the deprecation error is thrown when when the parser is doing its work. Since we cannot without to much effort get into that part of twig seems we cannot really do much.

May be a way out if we would load the druapl extentions before twig core extention, but that would mean maintaining the full contructor.

Hmmz. Can't we fix the debug output to not add newlines before/after or something? Or is there way more places where this will become an issue?

Well, it does work, but since the old one is loaded anyways before we replace it in the visitor it does actually trigger the deprecation. So not sure if we can even fully disable the deprecated one in CoreExtentions and have no error.

This might mean we need to actually replace the tag.

Ok, that approach will need more work. Was hoping for a quick one. Don't want to dive into my debugger right now and see when we can inject or override somehow.

Personally i would also rather use it only when needed like longwave suggests. There are quite a few places where it could be quite harmless to remove.

Made a quick MR to test replacing the filter.

We might be able to something like we do in 📌 Fix "Twig\Node\Expression\FilterExpression" deprecation introduced in twig/twig 3.12.0 Active then and override the spaceless filter.

This is how it is called in Twig itself:

new TwigFilter('spaceless', [self::class, 'spaceless'], ['is_safe' => ['html'], 'deprecated' => '3.12', 'deprecating_package' => 'twig/twig']),

If we need to keep it, I wonder if there is a way our then by overriding the spaceless tag so we don't trigger the deprecation. This would also be very helpfull for theres who might use it.

Draggable rows seem misaligned, more than that i cannot find.

ANother one

<div class="tabledrag-cell-content js-tabledrag-cell-content"><a href="#" title="Change order" class="tabledrag-handle js-tabledrag-handle tabledrag-handle-y"></a><div class="tabledrag-cell-content__item">Primary admin actions</div></div>

Height is wrong for the rows.

I'll do an install and fix the author. See if I can validate a few, just worried I cannot replicate all templates. Hopefully have time for that tonight.

Arg that's pretty annoying. We were hoping the fact is uses link template would remove all spaces around that.

Only one behind it it seems?

I also got nod to promise to take a look, he'll know what to expect.

DIsecting the change, you might actually be right. Instead of collecting ID's and then getting the entities we just reference the entity and create the list right away, doing away with the load multiple.

Technically its the same, there is also no risk involved with not load multiple imo since we know we have the entiy already. I cannot see how there could ever be a way to have different entities loaded with loadMultiple than we already know of in the (validated for existing) entity property.

This does assume the entity that is loaded is the correct revision, but that seems like ano brainer.

I want to RTBC, but:

1. IS needs update for new approach.
2. Need a change record describing the change

Added some credits. Commiter needs to decide on @mondrake

I've fixed all the issues timo and cees identified. Think every single change has now been reasoned about. Ready for review again.

Adding credit for 2 colleagues who went through it all.

Ty all.

https://www.drupal.org/project/jsonapi_extras/releases/8.x-3.26 →