🇳🇱Netherlands @bbrala

Added missing modules. Fingers crossed.

Think i missed a few modules to enable while mergin. Cant look right now but pretty sure thats why we have a failure left.

This issue was mentioned in slack in another context, thought i'd revive this. The rebase was a little hard with the changes done by now. At least phpstan is happy by now (and baseline did shrink a bit ;)). Lets see how it goed.

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

bbrala → created an issue.

Rebasing took a little effort because we need to deprecate some extra arguments. I also had to undo contructor promotion since the merged json:api metadata issue changed the contructor parameter.

I think the changes are small enough no to warrent a new review, all is green still.

Oopsie, not sure why we didnt see this before, but the atrtribute is set AFTER the $row[$key] was overwritten with an render array. I've fixed that.

I also applied/committed the suggestions. Thanks for having a look.

Still an open questino in #27 regarding the 'others'. This would be a non issue if one first saves before we commit. But that does seem not very user friendly.

Sorry for the noise, tool a random issue to check something. Please ignore.

Ty dww.

We do need something to decide what email is to be used. In the related issue it tells us that we cannot know user vs commit in gitpab, but wouldnt commits show up as gitlanId-userUid@git.drupal.org or something? That seems good enough to couple? Or is the problem the fact you can set any mail or username in your local git?

Not sure i see the problem.

Except we need to know what the correct email address is.

THanks, did some quick checks in the car. Think we can use $GLOBALS['user']['extern_uid'] which should be the git user id. [ID]-[username]@git....

Still though we could have a look at getting the public user email from gitlab. That seems doable. Only question then becomes speed since we would need to save that somewhere. But perhaps we should be more sane in the hook that actually fills that field.

Dunno, perhaps tonight ill find some time to continue;

Ah, sure. Though since it was adjustments it would be ok.

But a small rewrite is fine.

We might need a Committed-By: {currentUser}.

Also we need to decide what email adres to use. We could use a generic no-reply so mail adresses do not get added for all time even when people change their email adress visibility on d.o. But i do see real email adresses of committers in the commit log. So not sure what is wise.

Committed a first iteration of the implementation. If you want to test locally.

$ = jQuery;
// Hard overwrite for testing
Drupal.settings.drupalOrg.issueCreditMessageTemplate = "
            
              
              
              ✨
              Allow specifying `meta` data on JSON:API objects
                Needs work
              
             !type: !message\n\n!byAuthors\n!byReviewers";
Drupal.settings.drupalOrg.issueCreditTemplate = "git commit -m '" + Drupal.settings.drupalOrg.issueCreditMessageTemplate + "'";

Drupal.drupalorgUpdateIssueCredit = function () {
....COPY FROM MR
};
Drupal.drupalorgUpdateIssueCredit();

This should work on any issue.

Ok, i need to store my work unttil now, this is testable on here, will work more later. But don't wanna loose this hehe. Didn't end up using your code @fjgarlin sorry

Is uses new data- tags on the checkbox, have the php change for that ready. When you submitted 'patches' you are an author, otherwise a reviewer.

Todo:
Make sure the current committer/merger is in there
When category === feature requerst -> feat:
Wehn category === task -> task:
Others: chore: ? not sure yet.

$ = jQuery;
// Hard overwrite for testing
Drupal.settings.drupalOrg.issueCreditMessageTemplate = "
            
              
              
              ✨
              Allow specifying `meta` data on JSON:API objects
                Needs work
              
             !type: !message\n\n!byAuthors\n!byReviewers";
Drupal.settings.drupalOrg.issueCreditTemplate = "git commit -m '" + Drupal.settings.drupalOrg.issueCreditMessageTemplate + "'";


Drupal.drupalorgUpdateIssueCredit = function () {


  // Hardcoded changee to checkbox until php is testable
  $('#drupalorg-issue-credit-form input[type=checkbox][id^=by-]').each(function(){
    let $ele = $(this);
    let numbers = $ele.parents('tr').find('td');

    let patches = $(numbers[2]).clone()    //clone the element
      .children() //select all the children
      .remove()   //remove all the children
      .end()  //again go back to selected element
      .text();

    let files = $(numbers[4]).clone()    //clone the element
      .children() //select all the children
      .remove()   //remove all the children
      .end()  //again go back to selected element
      .text();

    let comments = $(numbers[6]).clone()    //clone the element
      .children() //select all the children
      .remove()   //remove all the children
      .end()  //again go back to selected element
      .text();

    $ele.attr('data-patches', patches);
    $ele.attr('data-files', files);
    $ele.attr('data-comments', comments);
  });


  var addCredit = $('#drupalorg-issue-credit-form input[name=add_credit]').val(),
    message = $('#drupalorg-issue-credit-form input[name=message]').val(),
    byHtml = [],
    byAuthor = [],
    byAuthorMsg = [],
    byReviewer = [],
    byReviewerMsg = []
  ;

  // Collect authors and reviewers.
  $('#drupalorg-issue-credit-form input[type=checkbox][id^=by-]' + (Drupal.settings.drupalOrg.isMaintainer ? ':checked' : '.saved-by-maintainer')).each(function () {
    let $this = $(this);

    // Already mentioned as author.
    if (byAuthor.indexOf($this.data('by')) !== -1 || byReviewer.indexOf($this.data('by')) !== -1) {
      return;
    }


    let userName = $this.data('by');
    if ($this.data('patches') && $this.data('patches') > 0) {
      byAuthor.push(userName);
      byAuthorMsg.push('Authored-by: ' + userName + ' (' + userName + '@no-reply.git.drupal.org)');
    } else {
      byReviewer.push(userName);
      byReviewerMsg.push('Reviewed-by: ' + userName + ' (' + userName + '@git.drupal.org)');
    }

    byHtml.push($.trim($this.next('label').html()));
  });

  console.log(byAuthor);
  console.log(byAuthorMsg);
  console.log(byReviewer);
  console.log(byReviewerMsg);

  // By default, credit others as reviewers.
  if (typeof addCredit !== 'undefined' && addCredit !== '') {
    let creditOthers = addCredit.split(',').map($.trim);
    creditOthers.forEach(function (creditOther) {
      byReviewer.push(creditOther);
      byReviewerMsg.push('Reviewed-by: ' + creditOther + ' (' + creditOther + '@git.drupal.org)');
    });
  }


  console.log(byAuthor);
  console.log(byAuthorMsg);
  console.log(byReviewer);
  console.log(byReviewerMsg);

  // Fill out credit summary.
  if (Drupal.settings.drupalOrg.isMaintainer) {
    if (byHtml.length) {
      $('#drupalorg-issue-credit-form .credit-summary').html(Drupal.t('<strong>Giving credit to</strong> !credits', {'!credits': byHtml.join(', ')}));
    }
    else {
      $('#drupalorg-issue-credit-form .credit-summary').html(Drupal.t('Expand and select contributors to give credit.'));
    }
  }
  else {
    if (byHtml.length) {
      if (Drupal.settings.drupalOrg.isFixed) {
        $('#drupalorg-issue-credit-form .credit-summary').html(Drupal.t('<strong>Credit given to</strong> !credits', {'!credits': byHtml.join(', ')}));
      }
      else {
        $('#drupalorg-issue-credit-form .credit-summary').html(Drupal.t('<strong>Draft credit given to</strong> !credits<br><small><em>Credit is assigned when maintainers comment, and granted when the issue is fixed.</em></small>', {'!credits': byHtml.join(', ')}));
      }
    }
    else {
      $('#drupalorg-issue-credit-form .credit-summary').html(Drupal.t('A maintainer has not commented and given credit yet.'));
    }
  }

  // For non-maintainers, repopulate from checkboxes for commit message templating.
  if (!Drupal.settings.drupalOrg.isMaintainer) {
    byAuthor = [];
    byAuthorMsg = [];
    byReviewer = [];
    byReviewerMsg = [];

    $('#drupalorg-issue-credit-form input[type=checkbox][id^=by-]:checked').each(function () {
      var $this = $(this);

      // Already mentioned as author.
      if (byAuthor.indexOf($this.data('by')) !== -1 || byReviewer.indexOf($this.data('by')) !== -1) {
        return;
      }

      let userName = $this.data('by');
      if ($this.data('patches') && $this.data('patches') > 0) {
        byAuthor.push(userName);
        byAuthorMsg.push('Authored-by: ' + userName + ' (' + userName + '@no-reply.git.drupal.org)');
      } else {
        byReviewer.push(userName);
        byReviewerMsg.push('Reviewed-by: ' + userName + ' (' + userName + '@git.drupal.org)');
      }
    });

    // By default, credit others as reviewers.
    if (typeof addCredit !== 'undefined' && addCredit !== '') {
      let creditOthers = addCredit.split(',').map($.trim);
      creditOthers.forEach(function (creditOther) {
        byReviewer.push(creditOther);
        byReviewerMsg.push('Reviewed-by: ' + creditOther + ' (' + creditOther + '@git.drupal.org)');
        byHtml.push($.trim($this.next('label').html()));
      });
    }
  }

  // Fill out template. It has already been translated server-side.
  $('#drupalorg-issue-credit-form textarea[name=command]').val(Drupal.formatString(Drupal.settings.drupalOrg.issueCreditTemplate, {
    '!message': message.replace(/'/g, "'\\''"),
    '!byAuthors': (byAuthorMsg.length > 0 ? byAuthorMsg.join("\n") : '').replace(/'/g, "'\\''"),
    '!byReviewers': (byReviewerMsg.length > 0 ? byReviewerMsg.join("\n") : '').replace(/'/g, "'\\''"),
  }));
  $('#drupalorg-issue-credit-form textarea[name=command-message]').val(Drupal.formatString(Drupal.settings.drupalOrg.issueCreditMessageTemplate, {
    '!message': message,
    '!byAuthors': (byAuthorMsg.length > 0 ? byAuthorMsg.join("\n") : '').replace(/'/g, "'\\''"),
    '!byReviewers': (byReviewerMsg.length > 0 ? byReviewerMsg.join("\n") : '').replace(/'/g, "'\\''"),
  }));
};
Drupal.drupalorgUpdateIssueCredit();

Thanks for opening this issue, you've been on fire lately.

I was thinking, wouldn't this "just work" (tm) if you set field permissions? Although i'm a little weary if this is possible to allow post/path but then not view. But this might just work because of entity api.

function YOUR_MODULE_NAME_entity_field_access($operation, FieldDefinitionInterface $field_definition, AccountInterface $account, FieldItemListInterface $items = NULL) {
  $result = AccessResult::neutral();
  if ($field_definition->getName() == 'field_we_care_about') {
    if ($operation == 'edit' && !in_array('administrator', $account->getRoles())) {
      $result = AccessResult::forbidden();
    }
  }
  return $result->addCacheContexts(['user.roles:administrator']);
}

Just so we dont forget, we can help decidiing what tag to use with some sort of info like this cheatsheet: https://github.com/pvdlg/conventional-changelog-metahub#commit-types

esmoves → credited bbrala → .

larowlan → credited bbrala → .

An index on a (var)char > 176 will fail since it will run out of the max 100 bytes. Mostly referring to that.

Review was done, this added feedback. Setting to needs work.

Also; reroll no longer needed and there are tests. Removing tags.

IS is not up to date to the current approach it seems. Have not really reviewed code.

Hmmz, althoough i understand the need to do this, i want to note that finding the minor it deprecated in will be a little harden. Although the CR will still mention it, when making rector rules to fix deprecations it means always checking the CR. Sometimes to support the new and old way of a deprecation there is no easy way around that and you need to up the version to the version where it was deprecated.

This is not really a big issue, but a reality. This would also mean that when looking at deprecation messages you only get the major which might be an issue when digging into why a deprecation is triggering. Expecially in contrib. Steps to find out would then be;

find the line in core, git blame, find issue, (and maybe find CR) to see what minor broke this.

It's a trade off, but i understand what this would bring since it will lower the amount of adjustments and rework of deprecation messages.

As per #19 i'm setting this to needs work.

As it needs test, and IS needs updating and the added test doesn't fail on test only right now this is still needs work.

Made https://www.drupal.org/project/test_commit_message → with you all as maintainers.

I've looked at how this could work and one thing that will become nasty is that eventuelally the default MR title cannot be changed in gitlab itself it seems. See https://gitlab.com/gitlab-org/gitlab/-/issues/16080

This is an issue, since when we move onto gitlab issues, this means MR's will end up with:

Resolve "Fix tests and translations" Which is the default. This is maybe even broken in the docs of gitlab right now. Since that states: "Fix tests and translations".

So things are getting harder every minute while looking at this to do this all in there.

We could still do for the squash message:

%{title}

Committed-by: %{merged_by}
%{co_authored_by}
%{reviewed_by}

(see https://git.drupalcode.org/help/user/project/merge_requests/commit_templ...)

Another possible problem is when we don't have all people in the MR, there mightve been a new version, or they only contibuted in the issue itself. Then we need to manually add them also.

So there are quite a few holes need need to work out. Or don't try to rush to the end of the line and start with d.o. @dww has a point with his steps. But until we have proper credits/issues in gitlab, we cant completely cover it there (or know what we are missing).

How scary is this for possible indexes? Not sure what field types we use but the character limit will go down as the bytes per caharacter go up. Jas this been considered?

Yeah same cocnclusion here. Thanks!

Hmm, this would mean a change in the returned format:

public const string DateTimeInterface::ISO8601 = "Y-m-d\\TH:i:sO";
public const string DateTimeInterface::RFC3339 = "Y-m-d\\TH:i:sP";

The only change is the colon (:) between the hours. This is fine for new defaults. Existing configuration should not be affected.

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

This is awesome, thank you so much. Looking good :)

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

This seems relatively low risk seeing how long this has been supported.

Thanks for this.

I wonder if we should to the BC way, supporting both annotaatinos and attributes. But perhaps since we are the source of the manager only supporting getting things by annotations is fine. Do you agree?

Hmm, fair enough. But what if a paragraph is linked to another entity? This would mean delete for all. That feels dagerous.

Reading the code im a little confused why the ->delete was added. Wasn't it deleting before but only unlinking?

Lets start with retargetting.

This makes a lot of sense. Made an MR and setting RTBC for now.

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

Fixed in 📌 Automated Drupal 11 compatibility fixes for paragraphs_edit Needs review

Ran through the changes, looking good. Did a small fix on cspell for an abvious spelling error.

Merging this so we unblock others.

Related issue (pretty much same): ✨ Drupal 11 compatibility and gitlab Needs review

nod_ → credited bbrala → .

Alwyas love beginning with tests to prove something. :) ill keep an eye out.

Lets help out.

The out of scope changes are because the branch was made from another base than the current target. When you make the branch make sure you branch off the right place. It is also possible to 'rebase onto' a new branch and fix the weird changes, but that kinda depends on a decent amount of git knowlegde. If the changes are pretty minimal, just make a new branch of 11.x and commit the same stuff again. You could then force-push the branch to the existing one and it should be a-ok.

I really appreciate the fact you made tests to proove the issue. This helps a lot! Thank you.

Ignore the part about the follow up, I switched around some thing mentally there.

Oops, guess my coffee didnt set in, then this could be closed i guess.

Adding needs developer tooling, we could have a rector rule for this. That should not be a blocker, but will help manage the work.

I've gone through the open threads and closed those that were awnsered. Only one open are all about the removed 'efficiency' optimization for setting original. I agree this could be a followup if we really want it. See 📌 [Follow-up] Investigate if we need to optimize EntityInterface::setOriginal() to only clone when not set Active .

Update the Issue summary to reflect the state of the MR. Also did a small tweak on the title of the change record.

Think this can be RTBC.

bbrala → created an issue.

I tried to create a new 11.0.x version of this patch, but there is something a little messy in there, perhaps something else did not get a backport, im not too sure. I'm not sharp enough right now to find out.

quietone → credited bbrala → .

The code looks good. The split between node and nodes did confuse me a little when seeing the namespace still there, but that does make sense.

Id opt to fix with deprecation fixes. Since, as longwace argues, its pretty small change.

+1 on rtbc

I agree, we should commit the fixes too if possible. Don't want the same as we did with 3.14.

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.