Problem/Motivation

Some of the projects were already moved to gitlab issue queues and the links to issues have a new format. Therefore the following code sniffing rules need adjustments:

  1. Drupal.Semantics.FunctionTriggerError.TriggerErrorSeeUrlFormat
  2. Drupal.Commenting.Deprecated.DeprecatedWrongSeeUrlFormat

Probably there are others.

Steps to reproduce

Add a deprecation notice to a method, use gitlab link there, e.g.:

  /**
   * Build the form to display for this plugin on the settings page.
   *
   * @param array $form
   *   The form to add the plugin settings to.
   *
   * @deprecated in ai_content_suggestions:1.4.0 and is removed from
   *   ai_content_suggestions:2.0.0. Instead use buildConfigurationForm.
   *
   * @see https://git.drupalcode.org/project/ai/-/work_items/3586437
   */
  public function buildSettingsForm(array &$form): void;

Code standards job in CI (or manual execution locally) will display a warning:

------------------------------------------------------------------------------------------------------------------------
 48 | WARNING | The @see url 'https://git.drupalcode.org/project/ai/-/work_items/3586437' does not match the standard:
    |         | http(s)://www.drupal.org/node/n or http(s)://www.drupal.org/project/aaa/issues/n
    |         | (Drupal.Commenting.Deprecated.DeprecatedWrongSeeUrlFormat)
------------------------------------------------------------------------------------------------------------------------

Proposed resolution

Add new format to support gitlab links

CommentFileSizeAuthor
#4 error-test.PNG10.07 KBangel_devoeted
#4 test_success.PNG4.34 KBangel_devoeted

Issue fork coder-3590649

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

a.dmitriiev created an issue. See original summary.

a.dmitriiev’s picture

Status: Active » Needs review
angel_devoeted’s picture

StatusFileSize
new4.34 KB
new10.07 KB

Tested the issue fork locally in a DrupalPod environment. When the GitLab URL is correct, the regex passes the tests fine:

success

Just a minor thing I noticed while testing—if a GitLab URL has a typo, the warning message still mentions only the old drupal.org formats, which might be a bit confusing for users now that GitLab is supported. Here is what it looks like:

error

It might be worth updating that fallback text in both Sniff files to include the new git.drupalcode.org format too. Also, I spotted a tiny typo in the test files where my_projectl has an extra "l".

avpaderno’s picture

For @deprecated tags, isn't @see supposed to link to a change record? That is what Drupal coding standards say.

klausi’s picture

Status: Needs review » Postponed (maintainer needs more info)

Good point @apaderno - all those @see links after deprecated tags and PHP warnings should probably link to change records. Will change records stay on drupal.org or will they also move to Gitlab?

The merge request looks good otherwise, but we shouldn't make it more lenient if change records are supposed to stay on drupal.org. Change records are also much better for users, as they don't have to read through long Gitlab issues and comments.

a.dmitriiev’s picture

I agree that such info should be in change records, true. But also the existing patterns include links to the issues:

// Allow for the alternative 'node' or 'project/aaa/issues' format.

. This is up to community of course. Maybe there are some other places where gitlab links should be allowed. Not sure what is the best approach here.

The MR also has change for the function trigger error. Is the link there also supposed to be a change record? Just asking for the best practice, I don't insist on this to be merged.

Thank you for feedback and review.

d34dman’s picture

It was not easy for me to figure it out.. so here is the a long version.

Problem

Once a project issues are moved to gitlab, it is not possible to access issues via drupal.org domain. The "issues" link on project page under Development heading redirects to Gitlab Work Items.

E.g. https://www.drupal.org/project/flowdrop#block-drupalorg-project-development

So as a maintainer, i can't even create a change record on drupal.org. The behaviour is by design, and even if i try to "hack" the system by going to a different project and select FlowDrop project (E.g. goto https://www.drupal.org/node/add/project-issue/rift_cq and change project to FlowDrop), i get a warning that "Issues are not enabled for this project. You will not be able to submit this form."

Workaround

Create a change record using this form https://www.drupal.org/node/add/changenotice

cmlara’s picture

Status: Postponed (maintainer needs more info) » Needs review

For @deprecated tags, isn't @see supposed to link to a change record?

Coding standards says:

%cr-link%

The link to the change record on drupal.org (for core deprecations) or the relevant issue.

The issue is a valid target under the current coding standards. If we believe only CR’s should be used coder should update itself to allow GitLab work items and raise a separate issue in coding standards to change the current rules, only after that is adopted should coder revert the work items allowance.

avpaderno’s picture

Truly, The link to the change record on drupal.org (for core deprecations) or the relevant issue. is not the correct definition of what %cr-link% is, since that would mean that contributed projects could just use a link to an issue, while contributed projects can have change records too.

cmlara’s picture

The link to the change record on drupal.org (for core deprecations) or the relevant issue. is not the correct definition of what %cr-link% is,

That was a quote from the coding standards page linked to in comment 5 describing %cr-link%

d34dman’s picture

I vote for "Work's as designed". Even though accidental, but this forces users (like me) to discover the change-notice route to create change records.

As @cmlara has brought to our notice, what we could do is to follow up on updating the guidelines on the text for %cr-link%.

cmlara’s picture

A key point IMO is that it is the community expectation is that the Drupal profile in coder adhere to the existing standards as written.

The tightened rule would be better under DrupalPractice, and even that might be debatable unless coder can show a strong community agreement that contrib modules desire to always use CR’s.

I could see maintainers placing a deprecation as a courtesy for non-api code (happens often in core) and if a CR were required a contrib module may just skip doing so. I may have even done it that way a few times myself IIRC.

I don’t have a strong opinion either way on what the final policy is, I draw concern with having to fight coder in the past on deviating from what is written and coder trying to use “our rules in coder are already…” to urge the coding standards team to change the text of written standards.

this forces users (like me) to discover the change-notice route to create change records

With the new GitLab system what I suspect you really want is the bot tooling message to make clear to users and maintainers where and how to create CR’s. The issue management page also likely could use an update to show the CR links. All of that is a Drupal.org Customization concern. Requiring maintainers to use CR’s won’t fix underlying UI/knowledge transfer faults.

avpaderno’s picture

How to deprecate / Constructor parameter additions contains the following sentences.

A change record is not required so use the link to the issue in the message.

I would rather use a change record, but that documentation page say it is not required.