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

Merge Requests

More

Recent comments

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Did some initial work. While walking thorugh this i kinda concluded posting patches also is still valuable. Easier to use to add to own work, but also easy to reuse some of the checks if a new patch is available. This would mean writing less login in git.

A quick test with push options didnt work unformately. Although i didnt dive to deep. Lets hope those will work, otherwise we will need to use the gitlab api, which is also fine, but another extra moving part unfortunately.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

True, might be less messy and complicated. Will end up deting some changes at some point though ;p

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

@mkindred if you tested, cant you RTBC then?

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

The flow could be

1. Try rebase
2. Rebase fails, make a new branch from origin, apply patch, force push.

See step 3 in the second set. The only moment this becomes a problem is if human start working in the same branch. Then things explode, or we need to create a new branch and clean mr, which could work.

We could jsut decide not to complicate things to much and keep it like that. If we are confused about the branch because someone committed there, AND the patch has changes, just make a new branch from origin, apply the patch and push to [branchname]-2 or something.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Just writing down how this could work. This assumed we add an SSH indentity to the issue bot, which is possible using secret variables in the repo.

Project update bot is started
When no issue present its easy imo:

  1. Create issue (already implementen)
  2. Click create issue fork
  3. Clone the issue fork
  4. Create a branch specifically named for the bot [issue]-project-update-bot
  5. Apply the patch
  6. Commit with message: "Automated Drupal 11 compatibility fixed - (run [RUN NUMBER)
  7. Push with push options. Seems the folliwng options are relevent: merge_request.create, merge_request.title="", merge_request.description="", merge_request.label="").
  8. This creates the branch and the merge request in one go.

When issue is present (with mr):

  1. check results branch for tag the patch was last modified.
  2. git log on the issue fork remote and search for a commit that ends with "- (run [RUN NUMBER TAG])" if it exists do nothing.
  3. git log on the issue fork remove and search for a commit that has "- (run ", this means there was a patch already applied. If the only commit is from issue bot. Clone the issue fork, add origin remote create a new branch from orgin, apply the patch and force push.

If there is other commits from other users it gets harder, since it might mean that there will be conflicts. The bot could try and apply the patch, but it will probably fail. Not entirely sure what the best workflow here is. Perhaps create a new branch? Or move the existing branch and then open a new one?

I'm not sure about that part of the workflow yet. Anyone have some insights.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Thanks everyone!

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

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

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Looks like its all good. It found existing issue (with patch) and then decided it was fine.

One thing we might need to do it give some sort of heads up that fixes provided by rector for Drupal 10 will require Drupal 10.1 and up. Although if composer/info.yml changes will be posted that will be kinda obvious.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Also, dryRun breaks the sql database for me locally, which took me a while to remember... fun :)

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Thank you so much for working on this.

Seems this is pretty close. In order to get things running i did need to update some packages. Tie ran out, but will fully test. Been testing on jsonapi_extras.

I'm a little worries about double issues, which seemed to happen at some point, so want to make sure that is fixed.

Pushed my WIP changes to 3413931-fixed-d11-wip-changes

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

(adding patch for internal policy reasons)

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

THink i fixed it, could use some help in reviewing. πŸ› Editor_file+Linkit: removing links broken Needs review

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Fix in related issue i think.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

The problem is as follows:

When an element has data-entity-type and/or data-entity-uuid, this module load that data into the ckeditor model. This means if another module adds those properties it will think its a file upload. This is wrong. The fix here is to check if the type is a file. Then load the model. There might be a world where it is better to add a new property like data-managed-by=editor_file, but im not sure of the side effects of this, and this might conflict on other levels.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

bbrala β†’ created an issue.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

I found a bug when using this with linkit and editor_file together, you cannot remove the link of an internal link. The href is removed but attributes are staying

    <a data-entity-uuid="8d3939ae-89a7-4d5b-acf4-caf039a5ed8e" data-entity-type="node">link to home</a>

I'll make a new issue.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Awesome, i'll test this.

The fact is gets double fixed is mostly because of the missing code in PHPStan, this will be fixed soon.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

The argument order is actually incorrect for d10 rules. Fun!

This means; AbstractDrupalCoreRector.php line 83 and 84 need to be switched. And then some tests will need to be updates.

The function:

     $result = DeprecationHelper::backwardsCompatibleCall(
        currentVersion: \Drupal::VERSION,
        deprecatedVersion: '10.3',
        currentCallable: fn() => Role::loadMultiple(),
        deprecatedCallable: fn() => user_roles(),
      );
 

The broken code in Drupal-rector:

    private function createBcCallOnCallLike(Node\Expr\CallLike $node, Node\Expr\CallLike $result, string $introducedVersion): Node\Expr\StaticCall
    {
        $clonedNode = clone $node;

        return $this->nodeFactory->createStaticCall(DeprecationHelper::class, 'backwardsCompatibleCall', [
            $this->nodeFactory->createClassConstFetch(\Drupal::class, 'VERSION'),
            $introducedVersion,
            new ArrowFunction(['expr' => $clonedNode]),
            new ArrowFunction(['expr' => $result]),
        ]);
    }

Thos arrowfunctions need swapping. The cloned node is the original node, the result is well, the result after rector.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

bbrala β†’ created an issue.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Merged and released in 0.19 :)

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

I'm gonna defer this review to another developer. ;x

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Hmm, yeah that is kinda breaking then. Wouldnt the correct path be adding an upgrade hook that adds the permission to the roles currently having that permission? Then its non breaking and can be disabled if you like.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Thanks for the cleanup.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

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

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Great work, thank you, dependency injection is a good idea. It could've been a new issue, but this is fine. :)

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

I tried to use this branch. Unfortunately i get an error:

bjorn@bjorn-pc:~/projects/la_eu$ ddev drush en default_content,la_eu_default_content

In SectionData.php line 34:

  Value assigned to "section" is not a valid section


Failed to run drush en default_content,la_eu_default_content: exit status 1
πŸ‡³πŸ‡±Netherlands bbrala Netherlands

bbrala β†’ created an issue.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Probably an good idea to be carefull in the coming months. The cycle this time is short and making the transition easy will help a lot in maintaining a positive view on the release cycle of Drupal.

A definitive set of rules on this will indeed be hard, i don't think we'd be able to make a full edgecaseless rule about this.

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

bbrala β†’ created an issue. See original summary β†’ .

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

hmm, autoload path is not correct then.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

bbrala β†’ created an issue.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

@mherchel there seems to be one line not removed from the document yet, a placeholder for example code, which seems to have examples right above it? Quite minimal imo, so not gonna remove rtbc.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Change record can be found here: [#3406417].

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

I can help dww. I was planning the same thing, but would've probably opted to make a temporary mirror of core and use some real people there. This would also help vor maintainers to check if they see weirdness.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Gonna set rtbc, and suggest it will be added to core meeting.

We doel still need a change record it seems though. Quietone did point at the correct project in #16

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Fixed in another issue.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Has been committeren.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Seems fixed. Thanks for checking

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Think this is fine, but i didn't do a local install so not sure if i can RTBC right now.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Thank you for picking this up!

You also need to update the expected message in Drupal\KernelTests\Core\Extension\ModuleLegacyTest which checks if we get the right messages.

See this page for the failure: https://git.drupalcode.org/issue/drupal-3405412/-/jobs/421338#L9161

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Would you like to have that information in the results? We should be able to know that pretty easily, but not sure how big that win is for the dashboard.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Nah we can kill it.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

As per discussion, merging.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Was just thinking to keep one as an example, it also had connected cases, and would mean you can see the overview at different places where those are shown.

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Not suer if this is automatable:

Call to deprecated method at() of class PHPUnit\Framework\TestCase: https://github.com/sebastianbergmann/phpunit/issues/4297 151 times

See #3217717: Replace usages of the at() matcher, which is deprecated β†’

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Removed few that are supported

πŸ‡³πŸ‡±Netherlands bbrala Netherlands

Fixed in πŸ“Œ Add rector for system_time_zones Fixed

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

bbrala β†’ created an issue. See original summary β†’ .

πŸ‡³πŸ‡±Netherlands bbrala Netherlands
Production build https://api.contrib.social 0.61.6-2-g546bc20