I created grammatically a service and configurations for a custom block, and refreshing the page would bring this error.
with this MR the error is gone.
looks good to me
RTBC
Locally the failed test passes, so I am unable to fix it, sorry.
moving to needs work.
I tryed to copy the changes to a branch from version 3.0.X ingoring the commits from 8.X-2.X
The file tests/src/Functional/UpdatePathTest.php doesn't exist in this branch.
I am not sure about automatic_updates.install because the automatic_updates_update_last_removed points to 9002 and this patch adds 9903.
Please review
lucienchalom → made their first commit to this issue’s fork.
Postponed until 2.x actually leaves alpha, so that we know which code actually needs to be marked deprecated.
shouldn't we keep as postponed?
I moved the deprecation error triger in classes:
3 core/lib/Drupal/Core/Asset/JsCollectionOptimizer.php
2 core/lib/Drupal/Core/Asset/CssCollectionOptimizer.php
and deleted the redundant trigger from
4 core/lib/Drupal/Core/Http/RequestStack.php
I am not really sure how to proceed with
5 core/modules/system/tests/modules/deprecation_test/src/Deprecation/DrupalStandardsListenerDeprecatedClass.php
6 core/modules/system/tests/modules/deprecation_test/src/Deprecation/FixtureDeprecatedClass.php
because they are "arbitrarily deprecated in order to test the deprecation error handling properties of DrupalStandardsListener."
and
1 core/lib/Drupal/Component/Plugin/PluginHelper.php
does not accept a contruct.
I tried to look for other classes deprecated but could not find any concrete, instantiated classes.
please help and review
lucienchalom → made their first commit to this issue’s fork.
I removed the changes aplied with phpcbf, please keep the issue scope.
i checked again with drupal check and there is still, or new, deprecations.
I fixed a couple, but don't know how to fix the rest:
------ ---------------------------------------------------------------------
Line hacked.drush.inc
------ ---------------------------------------------------------------------
:151 Function update_calculate_project_data not found.
💡 Learn more at https://phpstan.org/user-guide/discovering-symbols
:192 Function drush_print_table not found.
💡 Learn more at https://phpstan.org/user-guide/discovering-symbols
:206 Function update_calculate_project_data not found.
💡 Learn more at https://phpstan.org/user-guide/discovering-symbols
:280 Result of method Psr\Log\LoggerInterface::error() (void) is used.
:286 Result of method Psr\Log\LoggerInterface::error() (void) is used.
:351 Function drush_print_table not found.
💡 Learn more at https://phpstan.org/user-guide/discovering-symbols
------ ---------------------------------------------------------------------
------ ---------------------------------------------------------------------
Line src/Commands/HackedCommands.php
------ ---------------------------------------------------------------------
:99 Function update_calculate_project_data not found.
💡 Learn more at https://phpstan.org/user-guide/discovering-symbols
------ ---------------------------------------------------------------------
------ ---------------------------------------------------------------------
Line src/Controller/HackedController.php
------ ---------------------------------------------------------------------
:40 Function update_calculate_project_data not found.
💡 Learn more at https://phpstan.org/user-guide/discovering-symbols
:56 Function update_calculate_project_data not found.
💡 Learn more at https://phpstan.org/user-guide/discovering-symbols
------ ---------------------------------------------------------------------
------ ------------------------------------------------------------------
Line src/Controller/HackedDiffController.php
------ ------------------------------------------------------------------
:32 Parameter $entity_comparison of method
Drupal\hacked\Controller\HackedDiffController::__construct() has
invalid type Drupal\diff\DiffEntityComparison.
:32 Parameter $entity_comparison of method
Drupal\hacked\Controller\HackedDiffController::__construct() has
invalid type Drupal\diff\DiffEntityComparison.
------ ------------------------------------------------------------------
------ ---------------------------------------------------------------------
Line src/hackedProject.php
------ ---------------------------------------------------------------------
:84 Function update_calculate_project_data not found.
💡 Learn more at https://phpstan.org/user-guide/discovering-symbols
------ ---------------------------------------------------------------------
------ --------------------------------------------------------------------------
Line src/hackedProjectWebFilesDownloader.php
------ --------------------------------------------------------------------------
:15 Access to an undefined property object::$existing_version.
💡 Learn more:
https://phpstan.org/blog/solving-phpstan-access-to-undefined-property
:109 Method
Drupal\hacked\hackedProjectWebFilesDownloader::archive_extract() has
invalid return type Drupal\hacked\Archiver.
------ --------------------------------------------------------------------------
[ERROR] Found 14 errors
lucienchalom → made their first commit to this issue’s fork.
Patch and MR are the same, the t() calls were fixed without any problem.
Looks good to me.
RTBC
lucienchalom → made their first commit to this issue’s fork.
I am so sorry, I forgot about consequences of renaming a file!
Thank you bot.
Fixed now.
Thank you, I updated the name and function based on review.
Please review again?
Thank you for the update.
There are still t call in these files:
rocketship_paragraphs/modules/menu_clickthrough/src/Menu/MenuTreeStorageMenuHelper.php
rocketship_paragraphs/modules/menu_clickthrough/src/Plugin/Field/FieldWidget/ClickthroughWidget.php
rocketship_paragraphs/modules/menu_clickthrough/src/Plugin/Field/FieldFormatter/ClickthroughFormatter.php
Back to needs work
neither of the files needed the 'use Drupal\Core\StringTranslation\StringTranslationTrait;' because they extends code that already have it.
There are still some t() calls, I edited the Issue Sumary, with the list.
I did my best with descriptions and params documentation, please let me know anything I can make better.
needs review
Changed the comments based on the review, thank you!
MR !3 was focused in WeMegaMenuBuilder.php, but had a lot of rebases that afected other files.
MR !17 also focus on WeMegaMenuBuilder.php, without the bunch of files from rebase.
Neither has the WeMegaMenuBuilder.php clear for coding standards, so I keep it in needs work.
Added back the deprecation tag, the See tag after it will report a problem, because there is no link apropriated to it. The function was deprecated ouside a Drupal.org issue:
original commit
I also separate the upercase and lowercase fixes in a diferent commit, and tried to fix the most of the inconsistencies.
I hope its better now.
Please review
The test passed!
I moved the trait to 'core/modules/field/src/GetFieldLabelOptionsTrait.php'
A question, if moved to the Field module, will be available to be used in Field_Ui?
because it is used in Drupal\field_ui\Form\EntityViewDisplayEditForm
For some reason, all of the tests were failing on the main branch on my local enviromment, so I could not test if this change will work. but I think it will :D
folowing the issue sumary, the only problems remaining were cited in #7 and #19.
The MR is still okay and no new coding standard were added.
moving to RTBC
Hi lauriii, I tryed to aplly the trait, tell me if I did it right?
the next step would be remove the @todo and create the followup?
lucienchalom → made their first commit to this issue’s fork.
To make the text were we had FormClass more redable, I changed a couple of things. Looks like this now:
This plugin are used to abstract the concepts implemented by EntityPlugin.
In Entity API we have the possibility to link entity form 'handlers' to a specific form class, but the operation name and routes linked with it are very arbitrary and unpredictable specially in custom entities cases.
In that plugin you have the possibility to map operation and others useful information about entity to reduce complexity of retrieving each possible cases.
The interface description I changed to "An interface to get and return information on form modes."
And for "class node", "class term" and "class user" I changed to "Creates ___ routes".
Are those better?
Thank you for reviewing.
lucienchalom → made their first commit to this issue’s fork.
I ve run Project Upgrade Status → and everything looks good.
I adressed #11, please review
I would like to point comments #23
I fixed all that I could.The warnings about @see from the deprecated tag refers to the commit where the deprecated tags were add, but there is no issue on the module page related to that. That is why they doesn't match the standard.
please review
I opened a MR, and I am working in updating the last patch to it.
The test started to pass again, but there is no need to add a list of modules in the protected static $modules.
also, I removed the @todo from it. And the two test.txt send in the branch.
Please review
lucienchalom → made their first commit to this issue’s fork.
"Ordered lists use "1", for easier updates and to avoid errors"
So please fix in the Installation and Configuration sections?
thank you
I returned the test, its listed in the issue
#3296336: Re-add drupal/jquery_colorpicker in req-dev and tests once a D10 version is available →
so lets not remove it.
I also reverted a change in the test so it wont break in the issue anymore.
There were 4 more phpcs errors, i fixed one about null coalesce operator, but
FILE: .../image_effects/tests/src/Functional/Effect/RotateTest.php
---------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 3 LINES
---------------------------------------------------------------------------------------------------------------------------------
258 | WARNING | Unused private method setFuchsia()
275 | WARNING | Unused private method setTransparent()
294 | WARNING | Unused private method shift90()
---------------------------------------------------------------------------------------------------------------------------------Those methods are used in this format
'corners_transform' => 'setTransparent',
so i left them in the code.
someone can review and confirm if this was the best approach?
lucienchalom → made their first commit to this issue’s fork.
My Drupal is in version 9.5.9
I installed via Git => git clone --branch '9.1.x' https://git.drupalcode.org/project/flipbook.git
Without any patch or change.
extended the module in the UI
acessed /admin/structure/flipbook/list
added a flipbook, with pdf and image.
when seeing the flipbook created I found another error:
Warning: Undefined array key "type" in Drupal\Core\Theme\ThemeManager->render() (line 328 of core/lib/Drupal/Core/Theme/ThemeManager.php).
but not the error mentioned here.
So i could not review the fix
Please provide more info for steps to reproduce the error?
I did a rebase on the branch, but I recomend a full review since some changes are ages old.
Thank you, the change requested was made, everything else looks good to me
RTBC =)
I added all the other maintainers to the readme file, as Earl Miles (merlinofchaos) is still in the official page I will not remove them.
I also changed the list number in the configuration to follow "Ordered lists use "1", for easier updates and to avoid errors" from
documentation. →
Please review
lucienchalom → made their first commit to this issue’s fork.
I changed the introduction, installation and configuration sections to look more like the exemples on the
documentation →
.
Please review
lucienchalom → made their first commit to this issue’s fork.
Ordered lists use "1", for easier updates and to avoid errors (see Configuration), also, markdown will automaticaly number the list if we do so.
Please change the list in Configuration section.
documentation →
and
example →
.
The sugestion was fixed, and the Drupal call was changed to a dependency injection,
RTBC
Fixed as requested.
all phpcs clean.
Thank you for the quick response.
Everything looks good to me, moving to RTBC
runing phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml,twig returns a clean terminal.
at src/NgLightbox.php line 111
- // @TODO, decide whether we want to try and support paths or to adopt routes
+ // @todo Decide whether we want to try and support paths or to adopt routes.but the full sentence is broke in two, the full sentence is
// Decide whether we want to try and support paths or to adopt routes
// like core is trying to force us into.
so it should have a captalized word but leave the period only on the second line.
everything else looks good to me,
Patch rerolled aplied cleanly, all the changes from previous patches are still there and no new "Line exceeds 80 characters".
looks good to me =D
I tested locally and could not find anything else that needs to be done.
the help_hook was created and show the README on the help page.
If any improvements can be made, would be on the readme file, so I consider this RTBC
Thank you!
all the coding standards were fixed, and all the tests passing.
Moving to RTBC
added the last dockblocks, hope its all right now. =)
There are still work to do, a couple of docblock are still empty.
I plan to keep working on it, but if someone can do it better, be my guest =D
lucienchalom → made their first commit to this issue’s fork.
I fixed some more CS problems, the unused variables and the "global constant should not be used"
lucienchalom → made their first commit to this issue’s fork.
Error only happens on drupal 10 and creates a WSOD, not only on the help page.
The patch fixed the error, so RTBC for me
The module still have another warnings on the same file, so I recommend a new issue to remove those warnings
I manage to fix most of the errors on test.
The last one that i couldn't fix i reverted the DI, so someone else can do it.
lucienchalom → made their first commit to this issue’s fork.
I fixed all that I could.
Thank you for waiting @elber.
The warnings about @see from the deprecated tag refers to the commit where the deprecated tags were add, but there is no issue on the module page related to that. That is why they doesn't match the standard.
I did a reroll from the last patch, but there are more errors to be fixed, sorry.
Te tests are passing and the DI was fixed. \o/
The remaining cs of
FILE: /.../modules/contrib/ng_lightbox/README.md
----------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------------------------------
17 | WARNING | Line exceeds 80 characters; contains 87 characters
----------------------------------------------------------------------------------------------refers to
compass compile --no-sourcemap --no-debug-info --force -e production sass/lightbox.scss
that I consider that should stay this way,
I just added Escaping Backticks to maintain as a code in the visualization and be shorter.
=)
There is still one error to be fixed
FILE: ...\modules\ng_lightbox\src\NgLightbox.php
----------------------------------------------------------------------
FOUND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
112 | WARNING | [ ] \Drupal calls should be avoided in classes, use
| | dependency injection instead
----------------------------------------------------------------------@charchil thank you for opening a MR!, that helps on reviews a lot.
please be careful with naming branches for a MR, the way it is now the branch 2.x already exists (because is the branch for this version) so git checkout -b '2.x' --track ng_lightbox-3152816/'2.x' cannot be applied.
We need to use something like git checkout -b '3152816' --track ng_lightbox-3152816/'2.x' instead.
Also, please don't break the line of a code in the README.md.
That line is meant to help people just copy and past the code on their terminals, but with the line break, it will also break in the terminal and therefore, not work as expected.
I tried to reroll and merge the las two patches as they had different changes.
The test are failing! I could not fix it, so keeping in needs work.
the error was fixed, than you