πŸ‡¬πŸ‡§United Kingdom @alexpott

πŸ‡ͺπŸ‡ΊπŸŒ
Account created on 27 June 2007, about 17 years ago
#

Merge Requests

More

Recent comments

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Committed 6ef6a00 and pushed to 11.x. Thanks!

Only committing to 11.x as per the CR. I'd be in main to commit this to 11.0.x and 10.4.x but the CR stops me from doing that. And I don;t feel this bug fix is urgent.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Moving to Webform as per #13

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Description is not a required argument in \Drupal\Core\Mail\Attribute\Mail ... I feel this is something that should be fixed in webform.

All it needs to do is something like

  if ($mail_module_name || $mail_plugin_id) {
    $t_args = [
      '@module' => $mail_module_name,
      '@plugin_id' => $mail_plugin_id,
    ];
    $requirements['webform_email'] = [
      'title' => t('Webform: HTML email support'),
      'value' => ($mail_module_name) ? t('Provided by the @module module.', $t_args) : t('Provided by the @plugin_id mail plugin.', $t_args),
      'severity' => REQUIREMENT_OK,
    ];
    if ($mail_plugin_definition) {
      $requirements['webform_email']['description'] = 
        $mail_plugin_definition['label'] .
        // Add the description if available.
        isset($mail_plugin_definition['description']) ? ': ' . $mail_plugin_definition['description'] : '';
    }
  }

Note the use of formattable markup is unnecessary here - we can rely on Twig's auto-escape.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Added some comments to the MR.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Committed and pushed 238082ab82 to 11.x and 09db6d0852 to 11.0.x and cfeb5be479 to 10.4.x and 187c2bf4b3 to 10.3.x. Thanks!

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Added a comment to the MR.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ
πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Committed and pushed 32a59501a9 to 11.x and fe34d77335 to 11.0.x and 62d49255b9 to 10.4.x and fdb36c43ab to 10.3.x. Thanks!

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

#2021161: Replace the fallback node listing with a list controller β†’ incorrectly replaced status with mark_type... because #2010672: Rename 'type' variable of theme_mark to 'status' β†’ had happened while it was being developed and obviously a re-roll was not careful enough.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ
πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

alexpott β†’ created an issue.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

@quak this situation is tested for by the added tests. Made another jwt module is getting the way. Can you list all the modules with jwt_* installed on your site? Thanks!

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Never remove old updates unless you also use hook_update_last_removed(). You can empty them out. But in this instance I would just add a new update calling the same code.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Because we need to be able to load configuration during config import before a module has been installed. NOTE: the AutolaodingStorage only applies to sync storage and not to active storage - so in my mind the risk is actually minimal and correct because it is exactly the time you expect code to change. FWIW there is no difference here if you try to import a config entity for a module and the entity class is not present. Once you run the import things are not going to work as you expect or want. It would be great if #2628144: Ensure that ConfigImport is taking place against the same code base β†’ and πŸ› Ensure that ConfigImport is taking place without outstanding updates Needs work had landed back when they were green years ago but people got far too used to being able to import incorrect versions config and began to see it as a feature rather than the bug that it is.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

It must fail gracefully.

I beg to differ. It must fail loudly and obviously. If you have removed code that your configuration expects this is a big error that Drupal soldier's on from time and time again causing more harm.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ
πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Committed and pushed 6afdada2f7 to 11.x and de3e9ac6b3 to 11.0.x. Thanks!

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

I've implemented @catch's suggestion. The only change is the the text at the top of the message is now wrapped in a div which feels correct to me anyway. Given all this is doing is removing a theme template and re-using one we have in core... setting back to RTBC.

Obligatory screenshot attached to prove that nothing has changed.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ
πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ
πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ
πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

alexpott β†’ created an issue.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

This project is using gitlabci if the PHPCS errors are not reported there then they are not errors.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Committed and pushed 5af340f to 11.x and c888511d5f to 11.0.x and 8faad47da5 to 10.4.x and 8d6cde7902 to 10.3.x. Thanks!

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Thought about this some more. The composer inspection options will not work easily at all because module not always equal to project (think sub modules). Therefore this issue is going to add a setting to prevent deletes and put the management of removing translations in the user's hands.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

I've added some comments to the MR that need addressing.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

We still don't have a subsystem maintainer review. I discussed with @catch and we agreed that this issue is really a duplicate of #2418219: Deprecate destination URLs that don't include the base path β†’ and should be postponed on that issue. Until destination URLs are consistent, fixing this will be impossible.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

I'm really concerned about false positives here. For example what happens if the base path is admin - the situation envisaged by #2417645: Change destination query string handling to break dependence on system path β†’ where destinations can have or not have the base path is untenable. I think we need to complete the work on #2418219: Deprecate destination URLs that don't include the base path β†’ which would end up having to fix this. Not sure what to do here.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

@rkoller - yes let's open a follow-up to fix image fields too.

Committed and pushed 74b6cedac2 to 11.x and 3960a5d417 to 11.0.x and 5b240ef43d to 10.4.x and 07c0011de5 to 10.3.x. Thanks!

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ
πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Committed and pushed e015f80827 to 11.x and 8a748ac031 to 11.0.x and 8f08ecc0e1 to 10.4.x and fee6f4e082 to 10.3.x. Thanks!

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

\Composer\InstalledVersions is actually what should be used :D

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

alexpott β†’ created an issue.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ
πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Given this is a test only change backported to 10.4.x - makes it easier to maintain 10 and 11 compatible modules and update your code.

Committed 0426157 and pushed to 11.x. Thanks!
Committed cb746fd and pushed to 11.0.x. Thanks!
Committed fb01cca and pushed to 10.4.x. Thanks!

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Updated IS to reflect most recent changes.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

@nicxvan the PHPStan warnings a bogus - we need to file an MR to fix \mglaman\PHPStanDrupal\Rules\Drupal\Tests\BrowserTestBaseDefaultThemeRule against that project. I'm really not a huge fan of that rule - testing tests is OTT in my opinion - especially when if you run the test it will tell you it is missing a default theme. Anyway... the quick fix here is to change the baseline and move on.

The reason tests are failing is because you extended the wrong class :) - fixed in my push.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

The tests which are being changed to set up UID 0 do not need this change and should be reverted. Once that's done this can be set back to rtbc.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ
πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

I've made some of the requested changes and added comments on some of the others. I think we need to work on the docs added by @quietone.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Also, because recipes allow default content, how would you deploy a recipe to a remote environment without then re-requiring the recipe on that environment's composer? Is the expectation that the recipes folder is a committed folder rather than package managed? I wouldn't be entirely opposed to that, but I am slightly scarred from npm/bower-asset vs commit libraries folder :)

Recipes should not become a content deployment mechanism. We need to improve default content tooling in the module and core so that this can happen.

Leaving recipes in the root composer.json then subjects them to being updated via composer update. Recipes are not intended to be updated and reapplied like this.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

I've fixed the 10.3.x MR and I've provided an 11.x that improves the exception message so it is less cryptic. The deprecation message for 10.x is also explicit now too.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Discussed with @catch and we agreed to backport this to 11.0.x and 10.4.x.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Committed a75df83 and pushed to 11.x. Thanks!

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

@tstoeckler the advantage of this approach is now we don't need to be concerned with enforcing which mime type guesser comes last. Also less services.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

I've added typehints and declare(strict_types = 1); but I think we should not set MR back from rtbc for such things, ff PHPCS / PHPStan cannot enforce the standard.

Also the majority of typehints I added were for hooks or callbacks which always feels a bit meh because we have no way of enforcing that (yet).

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

As a docs fix backported to 10.3.x

Committed and pushed 82a5a70103 to 11.x and e1b47d1d96 to 11.0.x and c66e4aae4b to 10.4.x and cf46b0dc11 to 10.3.x. Thanks!

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Added a comment to the MR... we need to think about contrib and custom that are using the abstract class. The MR comment contains a plan on how to cope with that.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Yes we do need to keep the default config hash - these are exports from real sites and so would be there.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

We need to add a change record to cover this change. Also we should mention tests because quite a few have had to change as a result of this change.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

I think we should consider being broader here. Ether string|\Drupal\Component\Render\MarkupInterface or string|\Stringable. I think I prefer the latter.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

I'm wondering whether adding a fallback guesser is really necessary and whether the best place to add the fallback is in \Drupal\Core\File\MimeType\MimeTypeGuesser::guessMimeType()

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Let's mark this a fixed then.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

This is a config issue and if this is being thrown then config_inspector should catch it and warn the user that the config that has caused the exception is incorrect. This error occurs when config_object has been specified against a non root element. The error is specific and correct.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Yes this module does cause the error. It is incorrectly assigning config_object to seomthing that should be a mapping

https://git.drupalcode.org/project/ckeditor_codemirror/-/blame/3.0.x/con...

Looking at core you see mapping for ckeditor plugins

# Plugin \Drupal\ckeditor5\Plugin\CKEditor5Plugin\SourceEditing
ckeditor5.plugin.ckeditor5_sourceEditing:
  type: mapping

While we might rollback the exception in Drupal 10.x it will be quicker a better to fix this in this module ASAP.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

This is happening because this module's config schema is incorrect.

https://git.drupalcode.org/project/ckeditor_indentblock/-/blame/8.x-1.x/... should be mapping or something other than a config_object.

Looking at core you see mapping for ckeditor plugins

# Plugin \Drupal\ckeditor5\Plugin\CKEditor5Plugin\SourceEditing
ckeditor5.plugin.ckeditor5_sourceEditing:
  type: mapping

I guess it is the same for ckeditor_codemirror

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

I've added some comments to address to the MR.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

This error indicates that type: config_object has been added in an incorrect place in the schema. I agree that the friendly thing to do here is to emit a deprecation in 10.x and leave a break in 11.x as this is not a great thing. The config that has config_object placed at the wrong level will be invalid according to its schema.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

The test needs to prove that the correct revision is rendered.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

I think we need to allow empty strings for parent as this should not be changing APIs.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

alexpott β†’ changed the visibility of the branch 11.0.x to hidden.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

And a branch for 10.3.x (and applies to 10.4.x too)

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Pushed branch to update to v2.0.4

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

alexpott β†’ changed the visibility of the branch 3458751-drupal-10.3.x-regression-10.3.x to hidden.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

alexpott β†’ changed the visibility of the branch 3458751-drupal-10.3.x-regression to hidden.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

@quietone nice sleuthing. I still feel that we should try to get the columns in the order they are in the index. Sorting by column name is incorrect because the order in the index is significant.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

alexpott β†’ changed the visibility of the branch 3156439-postgres-followup to hidden.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

@longwave pointed out we should be able to trust our APIs and not assrt against the internals - so removed the assertions on index structure.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Opened πŸ› Postrges implementation of introspectIndexSchema is incorrect Active to fix the postgres schema inspection issue - I think the postgres implementation is incorrect and sorting by column name makes the method less valuable.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

alexpott β†’ created an issue.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

This looks great now.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

removing the UUID constraint feels like a step in the wrong direction. NULL is never really a valid value for site UUID.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

@quietone opened πŸ› Fix index test in LocalesLocationAddIndexUpdateTest::testExistingIndex Needs review so marking this back as fixed.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

I think given we have to sort the index on Postgres the assertions are largely pointless.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

alexpott β†’ changed the visibility of the branch 3156439-db-table-localeslocation to hidden.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

alexpott β†’ changed the visibility of the branch 3156439-db-table-localeslocation-10.x to hidden.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Created a small MR to get the test to pass on Postgres... by just skipping the index inspection shenanigans there.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

@sukr_s as both @larowlan and I agree going to set this back to needs work. This issue should:
1. Deprecate setting progressive as part of the batch
2. Convert batch_process and _batch_process into a service
3. Add a way to process the batch progressively and non-progressively via the new service.

The reason the API did not work is because it didn't make sense. Making the current change here will cause problems due to

- it will affect the whole processing, so what if set A specifies 'progressive' and batch B specifies 'non progressive' ?

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

I'm not sure that a NULL description is any better then an empty string. Do we think that this part of the change is actually worth it?

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

We should add test coverage of ContentLanguageSettings::validateId()

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Committed 9ddbc2c and pushed to 11.x. Thanks!

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

If path is deprecated we should deprecate it. See https://www.drupal.org/about/core/policies/core-change-policies/drupal-d... β†’

Added some review comments.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Committed a0466c5 and pushed to 11.x. Thanks!

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

This looks great.

Committed 900d64b and pushed to 11.x. Thanks!

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

If https://github.com/Lullabot/php-webdriver/pull/10 lands then this issue can be about updating the the version constraint.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Added a value object to make it slightly simpler... so now a future update could do:

$skipped_update = \Drupal::service('update.update_hook_registry')->getSkippedUpdate();
if ($skipped_update instanceof \Drupal\Core\Update\SkippedUpdate) {
  return $skipped_update->toMessage();
}

You don't even have to pass in the module and update number to the function because we can use the same trick to determine them from backtrace.

Also the value objects are nice for Drush because then they can use the same message as core easily.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Okay got a 10.3.x and an 11.x branch in place. Plus we already have πŸ“Œ Use selenium/standalone-chrome instead of our chromedriver image Needs work open to clean this up.

Production build 0.69.0 2024