🇳🇿New Zealand @murrow

Adding test task

Further update to the summary problem section

@sokru, I have updated the issue summary with a proposed solution. The dependency injection part will disturb the code a bit, but I don't think the other tasks will have much impact. What are your thoughts?

Changing the version number

In the current build (8.0.0-alpha2), I see no implementation that uses Drupal's HTTP proxy settings. This version of Elasticsearch Connector uses elasticsearch/elasticsearch 8, which should support proxies through the ClientBuilder's setHttpClientOptions() method, as described here: https://www.elastic.co/guide/en/elasticsearch/client/php-api/8.15/http-client.html.

Is it correct to say that proxy support is missing from this module?

@abhishek_gupta1, this looks good to me. I have applied the patch and everything is working as expected. Many thanks

murrow → created an issue.

@TR Patch #42 works for me on our dev site and is passing PHPStan with Drupal Core at 10.2.1

Just confirming that #3 works for me also (using Drupal 10.1.7).

In web/core/modules/system/system.install at line 1315:

  // Check if the SameSite cookie attribute is set to a valid value. Since this
  // involves checking whether we are using a secure connection this only makes
  // sense inside an HTTP request, not on the command line.
  if ($phase === 'runtime' && PHP_SAPI !== 'cli') {
    $samesite = ini_get('session.cookie_samesite') ?: t('Not set');
    // Check if the SameSite attribute is set to a valid value. If it is set to
    // 'None' the request needs to be done over HTTPS.
    $valid = match ($samesite) {
      'Lax', 'Strict' => TRUE,
      'None' => $request_object->isSecure(),
      default => FALSE,
    };
    $requirements['php_session_samesite'] = [
      'title' => t('SameSite cookie attribute'),
      'value' => $samesite,
      'severity' => $valid ? REQUIREMENT_OK : REQUIREMENT_WARNING,
      'description' => t('This attribute should be explicitly set to Lax, Strict or None. If set to None then the request must be made via HTTPS. See <a href=":url" target="_blank">PHP documentation</a>', [
        ':url' => 'https://www.php.net/manual/en/session.configuration.php#ini.session.cookie-samesite',
      ]),
    ];
  }

Yes, I believe that this was covered in #3288313. @npaudyal001, could you confirm and then close this issue please?

Adding a list to cover "Review what HTML elements to add" task.

@npaudyal001, I think only a maintainer has permission to update the project's version control page. Could I leave this to you, please?

@npaudyal001 Do you need to update the project version control page to include the --binary option on git diff? https://www.drupal.org/project/lingotek/git-instructions →

@npaudyal001, this is looking good. I will take a look into the coding standards errors now.

@smustgrave, are you saying that the note (now removed) stating that the button tag had been added to adminTags was in the wrong place, incorrectly specified or shouldn't have been there at all?

Reversing out $content_metadata_entity. (A conversation for another day: shouldn't the metadata entities behave like other Drupal entities – i.e., with fields instead of "entity" attributes?)

Undoing bad "fixes". The Lingotek metadata entity doesn't use getters and setters, just raw objects. PHPStan complains about this, but you have to leave it as it is or refactor the entity itself. I tried using getters and it was a mistake. So, rolling back …

@npaudyal001 LingotekConfigurationTrait.php was a file I added and the discovered it was not needed for outside of a single file. I put the functions in the trait into that class as private functions. I'm not seeing it mentioned in the errors here:

That was not in my patch @prauat. I mentioned the issue in #60, but the fix that was applied afterwards just exacerbated the problem. IMO, the type hints need to be returned.

Button is in #58, @prauat

Removing whitespace

Adding IS details for "button" proposed by @CarlyGerard and @prauat.

Further updates, with some Unit tests. 23 tests failing.

Drupal\Component\Utility\Xss::needsRemoval() requires an array. But, the patch has removed string[]:

   /**
    * Whether this element needs to be removed altogether.
    *
-   * @param string[] $html_tags
+   * @param $html_tags
    *   The list of HTML tags.
-   * @param string $elem
+   * @param $elem
    *   The name of the HTML element.
    *
    * @return bool
    *   TRUE if this element needs to be removed.
    */
-  protected static function needsRemoval(array $html_tags, $elem) {
+  protected static function needsRemoval($html_tags, $elem) {
     return !isset($html_tags[strtolower($elem)]);
   }

That is breaking "core/modules/editor/src/EditorXssFilter/Standard.php", which wants string[].

Adding button element.

No comments from those making patches about #40 and #43 and the recommendation to support the button element? Is this issue correctly named or is it just a responsive image issue? Button is valid and ought to be included.

Thanks for this patch, @npaudyal001. I am almost there and should have a fresh patch coming over the next few days.

Further updates. I'm still making my way through the code with Drupal Check.

@npaudyal001, I've almost reached a stage where I need the tests to work in order to proceed with any confidence. That is blocked by 📌 Fix test failures in 4.0.x (missing schema definition, etc) Needs work , which seems a long way from reaching D10. Should I shift my attention to the tests in this branch and not wait on 📌 Fix test failures in 4.0.x (missing schema definition, etc) Needs work or do we continue with trying to sort out obvious issues here (underlying library changes, undefined parameters, annotations, etc) and hope that the test ticket catches up?

BTW, thanks for the drupalCI updates on cweagans. I could take a look at the PHP 8.2 build today to see what the problem is there.

A couple of questions here that might help me contribute a little better than I have been.

These tests are required for D10 (so PHP 8.1). This ticket appears to be for D9.5, but it is a blocker for #3288313. And there is also #3344857 that seems to cover this issue + a number of other test-related errors. D9.5 is EOL on 1 November. We also appear to have a D10 build issue that doesn't seem to be resolved in any of these tickets and relates to the composer permissions afforded the cweagans patch library.

My first question is, what is a plan of attack to get Lingotek to D10?

My second question is, given that there are roughly 7,000 phpstan errors thrown up on my D10 build (using Drupal check) on the tests alone and just 19 being thrown for the D9.5 build above and the matter of D9.5 EOL, is it meaningful putting effort into D9.5 at all?

Probably, I am missing something/a whole lot – apologies in advance.

I am on D 10.1.2 with PHP 8.2 using Drupal Check. Mostly, I am seeing annotation problems, but there are also some underlying library refactoring that is having an impact. I'm attaching an update to #44 that has some of these changes. Am I heading in the right direction or have aI gone too far?

I too have tested #4 this on alpha4 and it is working fine.

+1 RTBC