Netherlands
Account created on 8 December 2015, almost 9 years ago
#

Merge Requests

More

Recent comments

🇳🇱Netherlands bbrala Netherlands

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

🇳🇱Netherlands bbrala Netherlands

Yeah that should be a new issue.

🇳🇱Netherlands bbrala Netherlands

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.

🇳🇱Netherlands bbrala Netherlands

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();}");
🇳🇱Netherlands bbrala Netherlands

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".

🇳🇱Netherlands bbrala Netherlands

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.

🇳🇱Netherlands bbrala Netherlands

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 :)

🇳🇱Netherlands bbrala Netherlands

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

🇳🇱Netherlands bbrala Netherlands

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

🇳🇱Netherlands bbrala Netherlands

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.

🇳🇱Netherlands bbrala Netherlands

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

🇳🇱Netherlands bbrala Netherlands

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

🇳🇱Netherlands bbrala Netherlands

bbrala created an issue.

🇳🇱Netherlands bbrala Netherlands

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.

🇳🇱Netherlands bbrala Netherlands

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

🇳🇱Netherlands bbrala Netherlands

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

🇳🇱Netherlands bbrala Netherlands

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.

🇳🇱Netherlands bbrala Netherlands

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

  }
}
🇳🇱Netherlands bbrala Netherlands

;(

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

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

🇳🇱Netherlands bbrala Netherlands

@smustgrave can you force push again?

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

🇳🇱Netherlands bbrala Netherlands

Fun fact.

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

🇳🇱Netherlands bbrala Netherlands

Back to NR after rebase.

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

And lol steven ;x

🇳🇱Netherlands bbrala Netherlands

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
🇳🇱Netherlands bbrala Netherlands

Thanks longwave, marking as fixed.

🇳🇱Netherlands bbrala Netherlands

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.

🇳🇱Netherlands bbrala Netherlands

Upload all the images

🇳🇱Netherlands bbrala Netherlands

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

🇳🇱Netherlands bbrala Netherlands

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

🇳🇱Netherlands bbrala Netherlands

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

🇳🇱Netherlands bbrala Netherlands

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

🇳🇱Netherlands bbrala Netherlands

Very much vacation time it seems, all seems included.

🇳🇱Netherlands bbrala Netherlands

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.

🇳🇱Netherlands bbrala Netherlands

Hehe no worries drumm. :) New territory here

🇳🇱Netherlands bbrala Netherlands

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...

🇳🇱Netherlands bbrala Netherlands

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

🇳🇱Netherlands bbrala Netherlands

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

🇳🇱Netherlands bbrala Netherlands

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.

🇳🇱Netherlands bbrala Netherlands

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

🇳🇱Netherlands bbrala Netherlands

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.

🇳🇱Netherlands bbrala Netherlands

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.

🇳🇱Netherlands bbrala Netherlands

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.

🇳🇱Netherlands bbrala Netherlands

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.

🇳🇱Netherlands bbrala Netherlands

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.

🇳🇱Netherlands bbrala Netherlands

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

🇳🇱Netherlands bbrala Netherlands

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)

🇳🇱Netherlands bbrala Netherlands

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.

🇳🇱Netherlands bbrala Netherlands

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?

🇳🇱Netherlands bbrala Netherlands

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.

🇳🇱Netherlands bbrala Netherlands

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.

🇳🇱Netherlands bbrala Netherlands

Made a quick MR to test replacing the filter.

🇳🇱Netherlands bbrala Netherlands

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']),
🇳🇱Netherlands bbrala Netherlands

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.

🇳🇱Netherlands bbrala Netherlands

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

🇳🇱Netherlands bbrala Netherlands

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.

🇳🇱Netherlands bbrala Netherlands

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.

🇳🇱Netherlands bbrala Netherlands

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.

🇳🇱Netherlands bbrala Netherlands

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
🇳🇱Netherlands bbrala Netherlands

Added some credits. Commiter needs to decide on @mondrake

🇳🇱Netherlands bbrala Netherlands

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

🇳🇱Netherlands bbrala Netherlands

Adding credit for 2 colleagues who went through it all.

🇳🇱Netherlands bbrala Netherlands

I'm always a little scared about these. Won't this break others the other way around? Have other ecosystem modules been tested with this patch?

🇳🇱Netherlands bbrala Netherlands

Well that feature flag does nothing unfornately. :( Ah well.

🇳🇱Netherlands bbrala Netherlands

All green without most of the whitespace stuff. @longwave did raise the question if these changes could lead to layout shifts. I find that hard to know. Hopefully someone can say something smart about that.

🇳🇱Netherlands bbrala Netherlands

This is eventually going to break quite a few, if not the majority of custom and contributed Drupal themes.

Is this something that automation will be able to help with? Just a couple years ago, people were explicitly advised to use the filter, as the tag was going away.

Hmmz, yeah it is gonna be quite discurptive. Basically we need to remove the spaceless tags, which would be fine. Its easy enough to make a script that removes those tags in twig templates. So in a way this could be added to update bot to fix. Although it would be something "special" since it wouldn't be rector really. So what the correct place for that is, that is something that needs some thought.

🇳🇱Netherlands bbrala Netherlands

I intially throught it would also remove whitespace between html tags in the variable. Let's see how wrong i was :)

🇳🇱Netherlands bbrala Netherlands

Hmm, maybe. I could check but that will take some time. Sometimes the resulsting data might also have whitespace.

I'll try and make some time to go through each and try if can be removed.

Steps:

  1. Find candidate
  2. Remove and push
  3. Wait for tests
  4. Next

Unfortunately the test doesn't want to run locally for me. :(

🇳🇱Netherlands bbrala Netherlands

Tests are all green, and the only thing changes in the tests are a few newlines.

I can't RTBC anymore though. I'll go find someone for that :)

🇳🇱Netherlands bbrala Netherlands

Yay got all green, now just some small optimizations to minimize the changes.

🇳🇱Netherlands bbrala Netherlands

I will do an extra iteration, i think we could make tests work with only a very small change.

🇳🇱Netherlands bbrala Netherlands

I've done a very small update, replacing \n with PHP_EOL and seems tests are green. This is technically fully your solution just changes the newlines.

See: https://git.drupalcode.org/project/drupal/-/merge_requests/9665/diffs?co...

Will need to check if we have adressed all comments though, i'll get backl to that, or someone else might.

🇳🇱Netherlands bbrala Netherlands

Hmm, very weird.

What im thinking is it might be the fact that we have real vs character newlines. I tried to get the same error locally on that test, didnt workt.

We could try making it real newlines:

$expected = '<div class="link-item"><div class="link-url"><a href="' . Html::escape($url) . '">' . Html::escape($url_title) . '</a></div>\n </div>';

change to

$expected = '<div class="link-item"><div class="link-url"><a href="' . Html::escape($url) . '">' . Html::escape($url_title) . '</a></div>
 </div>';

or

$expected = '<div class="link-item"><div class="link-url"><a href="' . Html::escape($url) . '">' . Html::escape($url_title) . "</a></div>\n </div>";

And see if that resolves it.

Production build 0.71.5 2024