🇫🇷France @MacSim

Hi machin3,

thanks for the feedback and sorry for that bug,
I tested it on a no cache environment with a single account when I made the development.

I'll try to reproduce the issue next weekend and review the fix you mentioned in order to deliver a new release soon

No credit for my participation ? :’(

macsim → created an issue.

Updated MR to reflect what #14 said.
But since token service is not part of drupal core, tests can't succeed.
This could potentially move to Token issues. Anyway nothing would happen without the module being enabled.

Closing this issue as it's working as designed even though I don't understand why we are using $element['#machine_name']['field_suffix'] when it's not defined in field_ui.settings schema.

macsim → created an issue. See original summary → .

it could be "needs review" but we need maintainers to choose between drop drush 11 support or not

#24

I'm not sure I understand what you mean by general hooks, but I'm pretty sure these hooks could go in one of the other files suggested by #11 and #19.

If there was a NodeHooks file in a module I would expect to find hooks implementations related to nodes rather than "general hooks".

I don't think Node module would need to contain a NodeHooks file as Node.php already holds the entity logic ; not sure why the module would need to call it's own hooks rather than just add the code to the entity class.
By definition, hooks are a way for other modules to alter/add behaviors on something they don't own the logic for.

While in core and for most cases, EntityHooks should be fine, what you say is correct:
Several contrib / custom modules might end up using a NodeHooks file for a very specific logic related to nodes (I had something like that in my mind in #14, I do agree with #16 that, for some use cases, EntityHooks might need to be split into multiple more specific classes and we already have an example in #19 with UserHooks in Groups module).

So maybe we should keep appending Hooks to the classes name.
This will make autocomplete easier in the IDE when we want to automatically add a use statement.

Nah same problem than 🐛 Datepicker doesn't register relative dates Active
I didn't found a way to fix it yet

It might be because ids are missing in markdown.schema.yml

filter_settings.markdown:
  type: markdown.parser
  mapping:
+   id:
+     type: string
+     label: 'Filter ID'
    override:
      type: boolean
      label: 'Override'

We might have to add something like that to all markdown.parser_settings.* and maybe also markdown.extension_settings.*.

Since this file is only used during the module installation, we should use a hook_update somehow

After applying the MR34 patch, I was able to save my text format configuration before removing the patch.
I exported the config and got the following diff:

+  markdown:
+    id: filter_null
+    provider: filter
+    status: false
+    weight: -15
+    settings: {  }

while we should have:

+  markdown:
+    id: markdown
+    provider: markdown
+    status: false
+    weight: -15
+    settings: {  }

Didn't saw that issue before.
I opened 💬 Support to use the datetime-local element Active few weeks ago which seems to be a part of what has been done so far in here.
I do agree with #287 with the fact that this should be split in several sub-issues.

Tests are now fine with equal / less / greater and between operators on initial load.

It might be nice to also add tests checking that everything is ok after a form submission with updated values.

Seems good with single default value (equal / less / greater) and between min/max default values.

Needs review with and without default values on each those use cases.

Gotta also write tests.

I don't know why and if we have to open a new issue for that or not but when we set the field to use a "between" operator with min/max default values,
$form_state->getUserInput() doesn't have values for this field on initial load (whereas it does after the form submission, and it also does if we set the field to use a '>', '<', '=' operator with single default value)

+1 on #5
I don't see a reason to have the "administer views" permission available if the Views UI module is disabled

Patch #56 works on D11.1.3

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

About last comment in MR, another option would be to force a drush 13 minimal version which is compatible with drupal ^10.2.5 || 11.x-dev.
This way we can drop v2 generators versions and make v1 compatible with chi-teck/drupal-code-generator 4.x ( 📌 Update Generators to be compatible with chi-teck/drupal-code-generator 4.x Active )
Less code to maintain but the module would need to be ^10.2.5 || 11

macsim → created an issue. See original summary → .

Reverting the status to "Needs review" since the description might not be what you want ;)

macsim → created an issue.

Setting the status as RTBC since merge request job was successful

macsim → created an issue.

Setting the status to "Needs review" since the composer install problem raised in pipelines is already on 4.0.x

Dear reviewers,
please make sure that the commands are available on Drush 12/13 and still available on Drush 11.

Note for the maintainers: when you'll want to drop drush 11 support, you'll just need to remove the drush services declarations from search_api_solr.services.yml and modules/search_api_solr_admin/search_api_solr_admin.services.yml
You could also force a drush 12 minimum version by adding the following lines to your composer.json in order to raise a conflict alert if someone tries to use an earlier version:

"conflict": {
   "drush/drush": "<12"
}

macsim → created an issue.

Gitlab CI is now enabled on the merge request : https://git.drupalcode.org/project/ckeditor5_inline_quote/-/merge_reques...
After this will be merged on 1.0.x, coding standards will run on every branch and each commit.

The fact that CI pipelines are running on gitlab is enough to set that issue to RTBC.
New issues tickets should be created to fix errors and warnings reported by the CI tools when this will be merged.

macsim → created an issue.

Since ckeditor5.blockquote is coming from https://github.com/ckeditor/ckeditor5 there's no reason to open an issue on Drupal core side.
Either we can ask for that change on CKEditor5 side or improve and use CKEditor 5 Inline Quote → module (even if I think that this should be a core feature).

Therefore I am closing this issue.

Thanks for the review.
It's not as easy as I thought.
I think we will have to restart from scratch.

If the selected text representing the quotation is a full paragraph, we will keep using the <blockquote> tag, else that <blockquote> tag should be replaced by a <p> tag and the inline quotation should be wrapped in a <q> tag.

My bad I saw the "needs work" status and the patch and didn't realised it was already merged to 3.0.x

  - Syncing drupal/datetime_flatpickr (dev-3.0.x 9798387) into cache
Gathering patches for root package.
Gathering patches for dependencies. This might take a minute.
  - Installing drupal/datetime_flatpickr (dev-3.0.x 9798387): Cloning 9798387fdf from cache
  - Applying patches for drupal/datetime_flatpickr
    https://www.drupal.org/files/issues/2025-01-25/support-time-entry-in-bef-3501292-2.patch (Support time entries in FlatpickrDateBef filter plugin)
   Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/2025-01-25/support-time-entry-in-bef-3501292-2.patch

Handling --sync option would also be super sexy

Sorry for the looong delay
I kind of had abandoned this module.
It has been fixed on 2.0.x

Thanks smustgrave.
I applied your suggestions except for the first one since it's how it's done on other core KernelTests.
IMHO it will be easier to maintain the "form + tests" mix rather than to declare the form in another location even if it sounds prettier.

woops sorry for the status change ; for me it's RTBC
I leave "Needs review" until few more people test it

Tested it on a migration where I have bigint as source and string as destination since integer can't be the solution (SQLSTATE[22003]: Numeric value out of range: 1264 Out of range value for column 'sourceid1' at row 1)

Before patch:

vendor/bin/drush mim my_migration --sync
 [notice] Rolled back 74 items - done with 'my_migration'
 [notice] Processed 74 items (74 created, 0 updated, 0 failed, 0 ignored) - done with 'my_migration'

After patch:

vendor/bin/drush mim my_migration --sync
 [notice] No item has been rolled back - done with 'my_migration'
 [notice] Processed 1 item (0 created, 0 updated, 0 failed, 0 ignored) - done with 'my_migration'

Seems good to me

Added kernel tests for datetime-local

Seems good for me now (only tried with a single field though).
Review needed especially with a min/max exposed filter.

macsim → created an issue.

CI job is now ok
Merged on 1.0.x

 ------ ---------------------------------------------------------------------- 
  Line   src/Form/MicrowaveSettingsForm.php                                    
 ------ ---------------------------------------------------------------------- 
  91     Method Drupal\Core\Form\ConfigFormBase::__construct() invoked with 1  
         parameter, 2 required.                                                
 ------ ---------------------------------------------------------------------- 
 [ERROR] Found 1 error    

macsim → created an issue.

CI cspell job is now ok
merged to 1.0.x

macsim → created an issue.

PHPCS job is now successful
Merged to 1.0.x

Added a phpcs.xml to include PHPCS DrupalPractice standards. By default only the Drupal set of rules is used

macsim → created an issue. See original summary → .

macsim → created an issue.

Not sure we really need that JS lib... Can't we make that new lines split on PHP side directly?

macsim → created an issue. See original summary → .

Added a phpcs.xml to include PHPCS DrupalPractice standards. By default only the Drupal set of rules is used

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

Some ideas for that 4.0.0:

• Drop Drupal 9 support
• Add Drupal 11 support
• Add PHP 8 attributes support
• Implements hooks as OOP (that would drop support to Drupal < 10.3)

I'll be glad to help you on all those points but I would say that first of all we need to take care of #3506099 Fix issues reported by PHPCS 📌 Fix issues reported by PHPCS Active so we have a clean base to work.

Applied the MR on a 7.0.2 and a 7.0.5 with a 'now' value and '>' operator on a single date field.

Before patching:
On initial load, widget is empty and the offset condition is not applied on the view's results.
If I change the date and submit the form, it's all good:
the widget displays the date I picked and results are correctly filtered.

After patching:
On initial load, widget diplays the good date (today) but it's not applied on the view's results (I've got results from few days ago).
If I change the date and submit the form, I've got the same behavior than @agileware #9:
view's results are correctly filtered starting by the date I picked (tomorrow) but the widget displays the wrong date (today)

#13 Yes I wanted some feedback, thank you.

I updated the issue summary ; hopefully it is clear enough and will help resolving the issue.

Same problem than Datepicker plugin:
"Datepicker doesn't register relative dates" ( https://www.drupal.org/project/better_exposed_filters/issues/3470801 🐛 Datepicker doesn't register relative dates Active )

With the patch provided on the related core issue ( https://www.drupal.org/project/drupal/issues/3505318 💬 Support to use the datetime-local element Active ) the BEF "Datetime picker" plugin is now working for me but I agree with #15: I didn't found a way to define the date format of the input.

MR provides a fix for the ValueCallback() method.

Few thoughts:
- We should maybe find a way to fix it in a less-dirty way (ie. not with that ugly "if" condition)
- It only fixes the problem for the datetime-local element but datetime element also uses the 'html_datetime' format and would break the same way
- Actual tests are failing - one error has no link with what has been commited: Behat\Mink\Exception\ExpectationException: The string "The text format ckeditor5 has been updated." was not found anywhere in the HTML response of the current page. (https://git.drupalcode.org/issue/drupal-3505318/-/jobs/4297615) ; the other one from SettingsTrayBlockFormTest.php (https://git.drupalcode.org/issue/drupal-3505318/-/jobs/4297616) might need more code changes

Sorry quietone #8 I didn't realised I changed the version

Ok now I undestand why I've got this behavior no matter what I pass to '#date_date_format'
It's because of
$date_format = $element['#date_date_format'] != 'none' ? static::getHtml5DateFormat($element) : '';

protected static function getHtml5DateFormat($element) {
  switch ($element['#date_date_element']) {
    case 'date':
      return DateFormat::load('html_date')->getPattern();

    case 'datetime':
    case 'datetime-local':
      return DateFormat::load('html_datetime')->getPattern();

    default:
      return $element['#date_date_format'];
  }
}

and as cilefen said in #4, 'html_datetime' (ie. "Y-m-d\TH:i") is not a valid format to pass to \DateTime::createFromFormat()