Move automated tests to GitLab CI

You can confirm that the CI is running tests. See the results at https://git.drupalcode.org/project/rest_views/-/merge_requests/11/pipelines

When MR !11 is merged, you (the maintainer) can go to https://www.drupal.org/node/2858988/qa → and disable testing on the legacy DrupalCI, at least for the 3.0.x branch. I copied the link in the issue summary, GitLab CI → , from that page.

I do not like failing tests, so I added some commits to keep phpcs and phpstan happy. I will add details to the issue summary.

Thank you for this!

I fixed a t() warning I still need to test.

I also added four // phpcs:ignore comments because they are overriding core views methods so I'm not sure why those are throwing a warning still.

The main issue is now that phpcs is passing stylelint is failing because it can't find a css file.

Can we disable stylelint since this project has no css libraries?

I think this explains why phpcs does not flag those violations in the core views module: from core/phpcs.xml.dist:

  <!-- Only include specific sniffs that pass. This ensures that, if new sniffs are added, HEAD does not fail.-->

  <!-- Drupal sniffs -->
  <rule ref="Drupal.Arrays.Array">
    <!-- Sniff for these errors: ArrayClosingIndentation, ArrayIndentation, CommaLastItem -->
    <exclude name="Drupal.Arrays.Array.LongLineDeclaration"/>
  </rule>
...

It looks as though there are only three places where EXPORT_OPTIONS is used:

class ListExportFormatter extends FormatterBase {
  const VALUE_LABEL    = 'value_label';
  const KEY_VALUE      = 'key_value';
  const EXPORT_OPTIONS = [
    self::VALUE_LABEL => 'Export label and value separately',
    self::KEY_VALUE   => 'Export as key/value',
  ];
// ...
    $elements['export_format'] = [
      '#type'          => 'radios',
      '#title'         => $this->t('Export format'),
      '#default_value' => $this->getSetting('export_format'),
      '#options'       => array_map([$this, 't'], self::EXPORT_OPTIONS),
    ];
// ...
    $summary[] = $this->t('@options', ['@options' => self::EXPORT_OPTIONS[$export_format]]);

Really it should be done like this:

class ListExportFormatter extends FormatterBase {
  const VALUE_LABEL    = 'value_label';
  const KEY_VALUE      = 'key_value';
  const EXPORT_OPTIONS = [
    self::VALUE_LABEL => $this->t('Export label and value separately'),
    self::KEY_VALUE   => $this->t('Export as key/value'),
  ];
// ...
    $elements['export_format'] = [
      '#type'          => 'radios',
      '#title'         => $this->t('Export format'),
      '#default_value' => $this->getSetting('export_format'),
      '#options'       => self::EXPORT_OPTIONS,
    ];
// ...
    $summary[] = self::EXPORT_OPTIONS[$export_format];

The only problem is that I do not think you can use $this->t() in a const definition. You should be able to do it in a constructor, after calling the parent constructor. That is, use a class property (maybe a static one) instead of a const.

The main issue is now that phpcs is passing stylelint is failing because it can't find a css file.

Where do you see that? Or did you already do something to fix it?

I'm using this: https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr... →

When I run it I get this message:
PASS composer
PASS composer-lint
PASS phpcs
PASS phpstan
PASS eslint
PASS phpunit
FAIL stylelint
> at standalone (/gcl-builds/web/core/node_modules/stylelint/lib/standalone.js:261:43)
> Error: No files matching the pattern "../modules/custom/**/*.css" were found.
> at standalone (/gcl-builds/web/core/node_modules/stylelint/lib/standalone.js:261:43)
pipeline finished in 1.35 min

Although the actual tests are not failing so I can work on resolving that locally.

I'll take a look at your other comments.

@nicxvan:

Yes, using a function like that looks good to me. You are correctly using $this->t() on literal strings, and I expect the overhead of a function call is just microseconds. If I am wrong about that, then you can use the familiar pattern

function getExportOptions() {
  if (empty($this->exportOptions)) {
    $this->exportOptions = ...
  }
  return $this->exportOptions;
}

But I would not bother with that unless it is actually a problem.

One little complaint: the Coding standards → say,

Variables should be named using lowercase, and words should be separated either with uppercase characters (example: $lowerCamelCase) or with an underscore (example: <code>$snake_case). Be consistent; do not mix camelCase and snake_case variable naming inside a file.

The file already uses snake_case for some variables ($export_format and $form_state) and now it uses lowerCamelCase for others ($exportFormat and $exportOptions).

I cannot mark this issue RTBC, since I made the changes before the last two commits. But those two commits LGTM.

Good point, I usually leave items like form_state from core snake case, I've updated everything to camelCase.

I merged this in, thanks for your contribution!

That's the sort of change you make when you decide that the module is really your own. ;)

Oops, I did not mean to move this issue back to NR.

Automatically closed - issue fixed for 2 weeks with no activity.