๐Ÿ‡ช๐Ÿ‡ธSpain @manuel garcia

Account created on 5 December 2007, about 17 years ago
#

Merge Requests

Recent comments

๐Ÿ‡ช๐Ÿ‡ธSpain manuel garcia

Fixing a mistake, forgot to account for when we build the arguments for the command since I changed the form keys.

๐Ÿ‡ช๐Ÿ‡ธSpain manuel garcia

A bit of refactoring and renaming things:

1. Removed the strip- prefix from the strip options as its unnecessary (already in the strip option array).
2. Added an all option so this setting is explicit. This allowed for simplifying how we cook the argument later. inside applyToImage().
3. Adjusted the schema file accordingly.

๐Ÿ‡ช๐Ÿ‡ธSpain manuel garcia

Hello all, we were looking patch such functionality and found this issue, so collaborating.

Patch still applies cleanly and looks to work fine. I noticed that the schema file would need to be updated, so doing so in this patch.

๐Ÿ‡ช๐Ÿ‡ธSpain manuel garcia

Also ran into this today - in my case setting proper permissions on the files folder did the trick as well, so thank you @zigazou for sharing.

๐Ÿ‡ช๐Ÿ‡ธSpain manuel garcia

I'm also facing the same issue, running drupal-check 1.4.0 and Upgrade Status 4.3.1.

Here are the contents of the neon file:

cat /tmp/upgrade_status/deprecation_testing.neon
# FROM mglaman/drupal-check/phpstan/deprecation_testing.neon
parameters:
        tmpDir: '/tmp/upgrade_status/phpstan'
        drupal:
                drupal_root: '/var/www/html/web'
        parallel:
                maximumNumberOfProcesses: 0
        customRulesetUsed: true
        ignoreErrors:
                - '#\Drupal calls should be avoided in classes, use dependency injection instead#'
                - '#Plugin definitions cannot be altered.#'
                - '#Missing cache backend declaration for performance.#'
                - '#Plugin manager has cache backend specified but does not declare cache tags.#'

        # FROM mglaman/drupal-check/phpstan/base_config.neon
        reportUnmatchedIgnoredErrors: false
        excludePaths:
                - */tests/Drupal/Tests/Listeners/Legacy/*
                - */tests/fixtures/*.php
                - */settings*.php
                - */bower_components/*
                - */node_modules/*
๐Ÿ‡ช๐Ÿ‡ธSpain manuel garcia

Moving to review to trigger the testbot

๐Ÿ‡ช๐Ÿ‡ธSpain manuel garcia

Thank you for the report and patch, was about to open an issue for this :)

I went through the patch, and it is clean and on point. No more changes required as far as I can see.
I then applied it locally and the errors are gone, site looking functional as usual. Moving to RTBC.

It would be nice to get this in, Drupal 10.2.0 recommends using PHP 8.2 https://www.drupal.org/project/drupal/releases/10.2.0 โ†’

Drupal now supports PHP 8.3 and recommends at least PHP 8.2.

๐Ÿ‡ช๐Ÿ‡ธSpain manuel garcia

Thanks for suggestion. Views accordion doesn't currently ship any CSS, the styles are provided by the jquery.ui theme under modules/contrib/jquery_ui/assets/vendor/jquery.ui/themes/base/theme.css

For that to happen someone would have to write an integration with Olivero to add support for jQuery UI. jQuery UI has been marked โ€œEnd of Lifeโ€ by the OpenJS Foundation - so the likely hood of this happening is very low.

๐Ÿ‡ช๐Ÿ‡ธSpain manuel garcia

Thank you everyone who helped with this, I've now comitted the changes and cut a new 3.0.0 release: https://www.drupal.org/project/contact_permissions/releases/3.0.0 โ†’

๐Ÿ‡ช๐Ÿ‡ธSpain manuel garcia

Let's bring in the phpstan.neon file from the PR we worked on for ๐Ÿ› view_profiles_perms_test not compatible with Drupal 10 Fixed , it should take care of this warning: https://git.drupalcode.org/issue/view_profiles_perms-3406407/-/jobs/475158

Re @andrew.farquharson #8

Hi @Manuel Garcia, Since branch 2.1.x has core compatibility with Drupal 9 and 10 I am not sure if the .gitlab-ci.yml needs to be edited to test on Drupal 9 and 10 versions? I imagine that the scope of this ticket is to make changes only to the .gitlab-ci.yml. The functional test is now fixed so it should work on Drupal 9 and 10, now? Is there anything i am missing?

I'm not entirely sure if we need to customize our .gitlab-ci.yml file to be honest, I had a read of https://www.drupal.org/node/3356364 โ†’ and couldnt figure it out. You are right in that we should be testing with both supported Drupal core versions though.

๐Ÿ‡ช๐Ÿ‡ธSpain manuel garcia

We now have a passing 2.1.x branch, so we can proceed with this.

๐Ÿ‡ช๐Ÿ‡ธSpain manuel garcia

OK patch came back green, committed and pushed to 2.1.x - thanks!

๐Ÿ‡ช๐Ÿ‡ธSpain manuel garcia

Ok I have compiled the changes form the PR #6 into the attached patch, except the changes related to gitlabci which we will add on โœจ Configure GitLab CI Postponed .

๐Ÿ‡ช๐Ÿ‡ธSpain manuel garcia

Added PHPstan config file on https://git.drupalcode.org/project/view_profiles_perms/-/merge_requests/... to handle warnings https://git.drupalcode.org/project/view_profiles_perms/-/jobs/460383

$ php vendor/bin/phpstan analyze $_WEB_ROOT/modules/custom/$CI_PROJECT_NAME $PHPSTAN_CONFIGURATION --no-progress || EXIT_CODE=$?
 ------ ------------------------------------------------------------------------------ 
  Line   src/ViewProfilesPermsPermissions.php                                          
 ------ ------------------------------------------------------------------------------ 
  41     Unsafe usage of new static().                                                 
         ๐Ÿ’ก See:                                                                       
            https://phpstan.org/blog/solving-phpstan-error-unsafe-usage-of-new-static  
 ------ ------------------------------------------------------------------------------ 

https://www.drupal.org/docs/develop/development-tools/phpstan/handling-u... โ†’ for context.

๐Ÿ‡ช๐Ÿ‡ธSpain manuel garcia

thanks Andrew for the work on this, its nice to see the tests running on Gitlab CI!

On this ticket we will be committing only whats needed for Drupal 10 compatibility. Once we fix this ticket we will tackle getting the Gtlab CI
running on โœจ Configure GitLab CI Postponed

Regarding the error fixed by adding of the permission configuration (https://git.drupalcode.org/issue/view_profiles_perms-3406820/-/jobs/452805), I think that adding the config file is not the correct solution, as this a permission dynamically provided by view_profiles_perms module, which is what we want to test for. However lead me to investigate further, and I eventually landed on #2571235: [regression] Roles should depend on objects that are building the granted permissions โ†’ - see its change records:

So this may technically be a actual current bug in Drupal 10? Not sure to be honest, but we probably have to mimic how \Drupal\taxonomy\TaxonomyPermissions does things

๐Ÿ‡ช๐Ÿ‡ธSpain manuel garcia

Thanks, I'm learning about this as I go, but as far as I can tell the build failed with:
Unable to install modules: module 'view_profiles_perms_test' is incompatible with this version of Drupal core. - and its using Drupal version : 10.1.8-dev

So indeed this is a bug, we (I) forgot to upgrade to Drupal 10 the module we use in the test to setup things.

I've created ๐Ÿ› view_profiles_perms_test not compatible with Drupal 10 Fixed for the same, postponing this until we get that fixed, so we can get tests actually running on Gitlab.

๐Ÿ‡ช๐Ÿ‡ธSpain manuel garcia

Apologies @andrew.farquharson - i have now pushed an empty commit to rectify the commit mentions for this ticket.

๐Ÿ‡ช๐Ÿ‡ธSpain manuel garcia

@andrew.farquharson this is already fixed in the 2.1.x branch:

https://git.drupalcode.org/project/view_profiles_perms/-/blob/2.1.x/comp...

  "require": {
    "drupal/core": "^9 || ^10"
  }

https://git.drupalcode.org/project/view_profiles_perms/-/blob/2.1.x/view...
core_version_requirement: ^9 || ^10

I've just switched the default branch of the project to 2.1.x in order to avoid future confusion and tagged a new 2.1.1 release with this fix.

๐Ÿ‡ช๐Ÿ‡ธSpain manuel garcia

Sorry for messing up, doing things in a hurry is never a good idea. Thanks for the patch, committed and pushed to 2.1.x - Please verify and I will cut a new tag.

๐Ÿ‡ช๐Ÿ‡ธSpain manuel garcia

Thanks @ploplesc!

Could you please confirm whether this is working as expected for you in -dev version before tagging a new release?

I did that just now, my findings

  1. Removed the previous patch
  2. Switched to using dev-2.0.x
  3. Ran composer install
  4. Cleared caches
  5. Ran drush prie - got me a valid link, serving a valid csv file.
  6. Ran drush prii no errors but i didn't try actually importing a file.

No errors are coming up if I run drush status or anything similar.

๐Ÿ‡ช๐Ÿ‡ธSpain manuel garcia

I've just cut a new 2.1.0 โ†’ release. Added the composer.json file and also we are dropping Drupal 8 support.

Thank you all for the help and patience in this.

https://git.drupalcode.org/project/view_profiles_perms/-/commit/fe151a20...

๐Ÿ‡ช๐Ÿ‡ธSpain manuel garcia

Thank you very much @AstonVictor

๐Ÿ‡ช๐Ÿ‡ธSpain manuel garcia

There is a 2.0.x branch with Drupal 10 compatibility. Any bugs related to the upgrade should be logged against that branch.
A 2.0 release would be in order, since Drupal 9 is no longer supported.

๐Ÿ‡ช๐Ÿ‡ธSpain manuel garcia

Forgot to update the namespace on previous patch...

๐Ÿ‡ช๐Ÿ‡ธSpain manuel garcia

Updated the patch with the change in directory.

๐Ÿ‡ช๐Ÿ‡ธSpain manuel garcia

I tested the #2 patch, verified the namespace and its the correct fix. The error is also gone once the change is applied.

@Ali_W, try verifying that the web/modules/contrib/migrate_tools/src/Drush/MigrateToolsCommands.php file is actually there perhaps.

๐Ÿ‡ช๐Ÿ‡ธSpain manuel garcia

Updating the title as this isn't just PHP 7.4.
Taking the leap here and moving to RTBC, it is being reported as valid, patch is small and looks good to me. Thank you @godotislate.

๐Ÿ‡ช๐Ÿ‡ธSpain manuel garcia

Just bumping this to get the maintainers attention. Drupal 9 EOLed on November 1st and the patch is simple and good to go.

๐Ÿ‡ช๐Ÿ‡ธSpain manuel garcia

Drupal 9 EOL is just around the corner (November 1st).
The patch would get us Drupal 10 compatibility and is very simple. RTBCing.

๐Ÿ‡ช๐Ÿ‡ธSpain manuel garcia

Here are the results of running the upgrade status tool on the project:

$ drush us-a jquery_countdown_timer
 [notice] Processing /var/www/html/web/modules/contrib/jquery_countdown_timer.

================================================================================
jQuery Countdown Timer,  8.x-1.3
Scanned on Fri, 10/20/2023 - 09:33

FILE:
web/modules/contrib/jquery_countdown_timer/jquery_countdown_timer.libraries.yml

STATUS         LINE                           MESSAGE                           
--------------------------------------------------------------------------------
Check manually 0    The 'countdown.timer' library is depending on a deprecated  
                    library. The core/jquery.once asset library is deprecated in
                    Drupal 9.3.0 and will be removed in Drupal 10.0.0. Use the  
                    core/once library instead. See                              
                    https://www.drupal.org/node/3158256                         
--------------------------------------------------------------------------------

FILE: web/modules/contrib/jquery_countdown_timer/jquery_countdown_timer.info.yml

STATUS         LINE                           MESSAGE                           
--------------------------------------------------------------------------------
Check manually 0    Value of core_version_requirement: ^8 || ^9 is not          
                    compatible with the next major version of Drupal core. See  
                    https://drupal.org/node/3070687.                            
--------------------------------------------------------------------------------

FILE: web/modules/contrib/jquery_countdown_timer/composer.json

STATUS         LINE                           MESSAGE                           
--------------------------------------------------------------------------------
Check manually 0    The drupal/core requirement is not compatible with the next 
                    major version of Drupal. Either remove it or update it to be
                    compatible. See                                             
                    https://drupal.org/node/2514612#s-drupal-9-compatibility.   
--------------------------------------------------------------------------------

All points picked up by the upgrade_status analisys tool have been addressed in the latest patch.

I have also gone over the changes on jquery_countdown_timer_init.js (which isn't picked up by the tool), they make sense and there is no other adjustments that we need to make there as far as I can see. To me the patch looks good to go so I am RTBCing.

Drupal 9 EOL is November 1st, which is just around the corner, it would be nice if we can get this in and cut a new release :)

๐Ÿ‡ช๐Ÿ‡ธSpain manuel garcia

hi @snap_x - just assessing where we are here (November 1st is coming very soon!), I had a look at your PR and it looks good, I only have one question though, the bot's instagram_lite.1.0.x-dev.rector.patch also removes the dependency on core's block module, should we do that?

๐Ÿ‡ช๐Ÿ‡ธSpain manuel garcia

Thank you for working on this, I went through the PR and it all seemed reasonable, +1 to RTBC. Would be great to get this in and cut a new release as D9 EOL is coming very soon โ†’ (November 1st!)

๐Ÿ‡ช๐Ÿ‡ธSpain manuel garcia

First, thanks all for the patch, I have been evaluating it and seems to work as advertised.

I had to reroll it so I am attaching the re-roll which should apply cleanly against 4.x.

De-assigning assuming this is no longer being worked by gbyte :)

๐Ÿ‡ช๐Ÿ‡ธSpain manuel garcia

Woot! #3359159: Add Status to OverviewTerms.php form was merged into core! Small step, but it improves the core UI when using this patch.

Hey thanks for that, a good improvement indeed!

๐Ÿ‡ช๐Ÿ‡ธSpain manuel garcia

Thanks very much for the offer @heddn, I have added you as a co-maintainer now.

๐Ÿ‡ช๐Ÿ‡ธSpain manuel garcia

Still needs work for:

And perhaps think of a different solution than just using the method from the trait for #45.7

๐Ÿ‡ช๐Ÿ‡ธSpain manuel garcia

Reverting the changes related to #45.7, apparently we cant use a method from a trait?

๐Ÿ‡ช๐Ÿ‡ธSpain manuel garcia

OK some more progress, did all I could but most likely someone else will need to help out here :)

#45.5 Had a look, but unsure how to do this,
RevisionRouteProvider::getRevisionRevertRoute() is already adding:

$route->addRequirements([
  '_entity_access_revision' => "$entity_type_id.update",
]);

Is this where we should do this?
#45.6 I agree, replaced with {@inheritdoc}
#45.7 I dont see why not, done (I hope)
#45.8 I don't know, left as is.
#45.9 - Same deal as for #45.5
#45.11 No reason that I can think of, fixed.
#45.12 I agree.
#45.13 Done.
#45.14 Dont know :)

๐Ÿ‡ช๐Ÿ‡ธSpain manuel garcia

Thanks for the review @Sam152!
Had just a bit of time, so here's just a bit of progress on that:

  • #45.1 - Yay, done (I hope)
  • #45.2 - Done
  • #45.3 - Possibly, changed to just using 'entity_revisions_table'.

Setting to needs review for the tests to run, but obviously this is still Needs work to address the rest of #45.

๐Ÿ‡ช๐Ÿ‡ธSpain manuel garcia
1) Drupal\FunctionalTests\Routing\RevisionRouteAccessTest::testRevisionRouteAccess
Unable to install modules entity_test, user, entity, block due to missing modules entity.

This should get the tests running.

๐Ÿ‡ช๐Ÿ‡ธSpain manuel garcia

Most conflicts are easy enough to fix, however core/misc/autocomplete.js has been changed by #2823589: Improve IME handling on Autocomplete โ†’ so I'm stepping away from this reroll in fear of messing things up :)

๐Ÿ‡ช๐Ÿ‡ธSpain manuel garcia

Rerolled, was just a simple conflict on core/misc/autocomplete.js

Production build 0.71.5 2024