🇬🇧United Kingdom @nexusnovaz

I was unsure what was meant by fatal error and couldn't find anything specific for loading exceptions so have gone with a normal \Exception. Please could MR !10260 be reviewed.

Thanks

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

Hey, I've made a start on this.

Hopefully im reading this right and understanding the situation. I would assume this just means that it generates the config as
module_name.entity_id if no config_prefix is passed in. Ill set to review in case its right. Please see MR !10258

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

I was trying to write some tests unfortunately I can't seem to get PhpStorm to work with it. I've rolled #12 into the MR !39. I'm going to keep trying to see if i can get mine to work, but if anyone gets it first, feel free.

nexusnovaz → changed the visibility of the branch 3032084-add-an-option to hidden.

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

Hi,

I believe I got quite a few of them, though its actually rather difficult to tell. For example: "The giftcard storage schema." should this be changed or kept the same? Same with "Access control handler for giftcard transactions.". I'm going to mark as needs review for the words, im unsure why the tests are failing. Something about views page 2 schema. If this isn't the correct workflow please let me know and ill make a note for future.

I also noticed another issue which seems out of scope for this ticket but im unsure how to go about creating a bug/ticket for it. In src/Entity/GitftcardType:76 there is `The the` in a comment.

Thanks

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

Hey,

Thanks for that. I wasn't too sure about the return type but also wasn't sure what the correct answer was. Thanks for the link to the coding standards for this issue too! Setting back to needs review before pipeline has run, but it should be good!

Thanks

Hey, can MR !10040 please be reviewed. Pipelines had a couple of fails, but after a retest they were fine.

Thanks!

I'll take a quick look at this

Hey @joachim,

Apologies, was waiting for the pipeline to pass/fail before changing and requesting code review on this! Could i confirm the workflow process? Is it When i start working on an issue, i assign to myself and leave as active? Or change to needs work?

Also, i believe MR !10007 is ready for review. There was a failed test but that seems unrelated to this issue, so im hoping a rerun of that test will fix it.

Thanks.

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

Hi @c.moisiadis, would you mind assigning credits for this issue.

Thanks.

Another issue with phpstan. has less information than #61

----------------------------------------------------------------------------------------------------

Running PHPStan on changed files.

PHPStan: failed

----------------------------------------------------------------------------------------------------

I am unsure how to fix this

An unrealted test was failing, but after a rerun, its all green now. Marking as needs review unless i've missed something else?

breidert → credited nexusnovaz → .

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

Hey, updated publishcontent.settings.yml to make ui_checkboxes true. Should the change to the .install be reverted back to 0?

Will mark as needs review in case this is all good. Thanks

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

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

I see now @smustgrave. Cheers and apologies!

Hopefully i understood #31 correctly. Added the small change! Please review

Converted patch #66 to an MR (!9533). Unsure where to start on tests, though i will be following

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

Hey, i fixed the test and had to reformat the exception message due to it printing ()->getLabel() rather than the actual label. It took some tests a couple of retries due to failures, but they passed in the end. Please could the MR be reviewed. Thanks!

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

Hey, made the CR which can be found here: https://www.drupal.org/node/3475054 → . Also replaced the issue links in the MR with this link. Please can this be reviewed again.

Updated deprecations to be 11.1.0. Could you please review MR !6742. Thanks!

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

Hi, MR !9498 should now be ready for review.

Thanks

Hey @joachim,

How should i mark the default? Would it be (default) before the option? Does the default need to be the first in the list? (default) at the end of the item? I couldn't find anything in the coding standards, unsure if maybe i've missed it.

Thanks

Hey, this is ready for review. Please see MR !9499.

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

Assuming I haven't misunderstood, this should be ready for review. Please see MR !9498.

I've added the (optional) before severity in the docblock.

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

Hey, this is ready for review, please see !9497.

I moved the watchdog message to just before $return = TRUE; as to not confuse failed crons (due to cron lock, etc.). However, this can be moved again if needed.

Thanks

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

Found two more files which matched the problem. There is one last file which is contained in ckeditor5-dll.js. As this wasn't a normal file, i've left that one to be discussed. There are another 100 matches however, those are xmlns references.

Ill take a quick look and see if i can get the rest

Resolved cspell and most phpcs issues. I'm unsure how to properly resolve the final issues.

Remaining phpcs issue:

FILE: ...tom/migrate_devel-3458112/src/EventSubscriber/MigrationEventSubscriber.php
--------------------------------------------------------------------------------
FOUND 10 ERRORS AFFECTING 10 LINES
--------------------------------------------------------------------------------
 33 | ERROR | Variable "Source" starts with a capital letter, but only
    |       | $lowerCamelCase or $snake_case is allowed
    |       | (Drupal.NamingConventions.ValidVariableName.LowerStart)
 34 | ERROR | Variable "Destination" starts with a capital letter, but only
    |       | $lowerCamelCase or $snake_case is allowed
    |       | (Drupal.NamingConventions.ValidVariableName.LowerStart)
 43 | ERROR | Variable "Source" starts with a capital letter, but only
    |       | $lowerCamelCase or $snake_case is allowed
    |       | (Drupal.NamingConventions.ValidVariableName.LowerStart)
 47 | ERROR | Variable "Destination" starts with a capital letter, but only
    |       | $lowerCamelCase or $snake_case is allowed
    |       | (Drupal.NamingConventions.ValidVariableName.LowerStart)
 68 | ERROR | Variable "Source" starts with a capital letter, but only
    |       | $lowerCamelCase or $snake_case is allowed
    |       | (Drupal.NamingConventions.ValidVariableName.LowerStart)
 69 | ERROR | Variable "Destination" starts with a capital letter, but only
    |       | $lowerCamelCase or $snake_case is allowed
    |       | (Drupal.NamingConventions.ValidVariableName.LowerStart)
 70 | ERROR | Variable "DestinationIDValues" starts with a capital letter, but
    |       | only $lowerCamelCase or $snake_case is allowed
    |       | (Drupal.NamingConventions.ValidVariableName.LowerStart)
 79 | ERROR | Variable "Source" starts with a capital letter, but only
    |       | $lowerCamelCase or $snake_case is allowed
    |       | (Drupal.NamingConventions.ValidVariableName.LowerStart)
 83 | ERROR | Variable "Destination" starts with a capital letter, but only
    |       | $lowerCamelCase or $snake_case is allowed
    |       | (Drupal.NamingConventions.ValidVariableName.LowerStart)
 87 | ERROR | Variable "DestinationIDValues" starts with a capital letter, but
    |       | only $lowerCamelCase or $snake_case is allowed
    |       | (Drupal.NamingConventions.ValidVariableName.LowerStart)
--------------------------------------------------------------------------------

Remaining phpstan issues:

 ------ --------------------------------------------------------------------- 
  Line   migrate_devel.drush.inc                                              
 ------ --------------------------------------------------------------------- 
  27     Function drush_get_option not found.                                 
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols  
  55     Access to constant DEBUG on an unknown class Drush\Log\LogLevel.     
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols  
  112    Access to constant ALERT on an unknown class Drush\Log\LogLevel.     
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols  
 ------ --------------------------------------------------------------------- 

Hey, I think this is ready for review. Please see the MR.

Hey! I'll take a look at this!

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

Marking this as needs work. MR !2 should fix most issues, just a couple more for javascript which i am less knowledgeable on. Will leave for someone else to pick up! Updated the issue summary to reflect remaining task.

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

Made a push to remove two unused use statements which is now passing phpcs which can be reviewed in MR !2. There is an issue with cspell which is erroring with:

Dictionary Error with (drupal) Error: ENOENT: no such file or directory, open '/builds/issue/link_description_attributes-3458115/web/core/misc/cspell/drupal-dictionary.txt'

. I'll mark as needs work for that issue.

Ill take a look at this!

Hey, setting this back to needs review. Please see MR !3

Hey, ill take a look at converting the patch into a PR.

NexusNovaz → created an issue.

Hey,

I think this is ready for review. Created a new form at the route: admin/config/media/sharpstream/process which will just present the user with options which they have entered in the config under customer ids. Upon selecting and submitting, it will loop through all items and process them.

Please could you review MR !3

I've created a simple form for now, with no functionality. Need to have a think about the API calls. Should we just process client ids? This is an example using randomly generated customer ids. We could move the process function to another class and call that with the options from the user checkbox. Unsure if we can do a date range.

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

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

Hi @aaron.ferris,

I believe this is all good now. The codebase will need to be updated to Drupal 10 before this can be tested, but after doing it locally, all looks good! Ill leave this as postponed for now while we wait on 📌 D10 compatibility Postponed

Ill take a look at this.

Hey.

This is ready for review. Added some eslint disable/enable comments to get around not beautifying the code provided by fullstory. Please see MR !3

Hey,

Ill take a look at this.

NexusNovaz → changed the visibility of the branch 3422604-patch-2-to-MR to hidden.

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

Hi @aaron.ferris,

This is done for D10 - 2.0.x. Please review MR !22. Solution is similar for D7, would need to install the transliteration module and change some functions. However, thats if we decide to continue support for D7.

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

Checked out the MR. Issue URLs have been updated to #2346329. Can't find any more instances of Issue #2225961 being referenced.

Marking as RTBC

Hey @aaron.ferris,

Thanks for that. Makes sense. Added config/install/menu_css_names.settings.yml file with the default settings and config/schema file for the settings. Since we will have these default settings, i've removed the ?? FALSE from the settings form.

Please could you review MR !16 again.

Hey, this should be ready for review. Please see MR !16

I'll take a look at this!

NexusNovaz → changed the visibility of the branch 3451780-coding-standards-mk2 to active.

NexusNovaz → changed the visibility of the branch 3451780-coding-standards-mk2 to hidden.

Hey,
This is ready for review. Please see MR !14

Thanks

Ill take a look at this!

Hi,

I've resolved the comments added to the MR and it should be good for review. Warnings on phpcs and cspell are unrelated to this ticket.

NexusNovaz → changed the visibility of the branch 3451606-placeholder---fix to hidden.

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

Pushed changes for phpcs. Should be all good to review now

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

Apologies!

Updated the location and the line of code. I believe it now works as expected. Please could !55 this be reviewed

I've added the line you mentioned above to the file. Tested on a vanilla drupal 10 instance and the first fieldset is marked as required. Though, i suspect another change may be needed to require the second fieldset.

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

Tested with Gin admin theme and Olivero. Can confirm it shows the classnames.

Hi @bbrala,

I'm unable to get the test to pass. I'm getting the following fail message with $url:

PHPUnit 9.6.19 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.1.27
Configuration: ./core/phpunit.xml.dist

Testing Drupal\Tests\jsonapi\Functional\NodeTest
.F..........                                                      12 / 12 (100%)

Time: 01:16.183, Memory: 6.00 MB

There was 1 failure:

1) Drupal\Tests\jsonapi\Functional\NodeTest::testGetIndividual
The 'errors' member was not as expected.
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
         1 => 'Forbidden'
         2 => 'The current user is not allow...ource.'
         3 => Array (
-            0 => Array (...)
-            1 => Array (...)
+            0 => '/data'
         )
+        4 => Array (...)
     )
 )

Or the following error message with NULL:

PHPUnit 9.6.19 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.1.27
Configuration: ./core/phpunit.xml.dist

Testing Drupal\Tests\jsonapi\Functional\NodeTest
.F..........                                                      12 / 12 (100%)

Time: 01:17.307, Memory: 6.00 MB

There was 1 failure:

1) Drupal\Tests\jsonapi\Functional\NodeTest::testGetIndividual
The 'errors' member was not as expected.
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
         1 => 'Forbidden'
         2 => 'The current user is not allow...ource.'
         3 => Array (
-            0 => Array (...)
+            0 => '/data'
         )
+        4 => Array (...)
     )
 )

As im unsure what to do, ill leave this for someone else and check up once complete to learn from the solution.

Hi, i think the Remaining tasks needs to be revised. I've removed the old test and uncommented the new one. The test can be viewed here with a failure.

Ill pick this one up

Hey,
This should be ready for review. Added the return type array to Facet::__sleep() in MR #211

Hey, Im on it!

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

Nice spot @ksenzee!

Made that adjustment to the MR and should be ready for review!

Hi, I've made MR 8124 which is ready for review. I've added both the patch from #9 and the changes in #16.

Thanks

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

Hi @joachim.

I've taken the doc string from the interface and replaced the inherit doc with that string. MR 138 is ready for review.

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

Hello!

I've made a MR to translate the four comments. Please review MR #32

Thanks