#Needs tests

I left a minor comment on the MR, which is a bit of a nitpick and I'm 50/50 whether it's in scope or not. Leaving at RTBC for now.

For the record, I just noticed this issue on one of my client sites. It was on Core 10.4.5. I updated to Core 10.4.6 and I'm still seeing the same issue. I will report back once I figure it out. I wanted to add this comment in case anyone is looking through issue queues.

Since steps aren't provided going to close out for now. If still an issue please re-open updating issue summary with core steps.

Re-rolling patch for 1.38.

I'm wondering if the last part of the patch is still needed since the module has a null check on getSearchApiQuery(). But regardless of that, I assume it still needs to check shouldAbort().

Test failure seems relevant to the change here.

prineshazar → changed the visibility of the branch 3199697-add-jsonapi-translation to active.

patch #19 works for me.

Here is the patch with the hacky approach.

I have rebased this MR and resolved the merge conflicts. During the rebase, I noticed that some new behavior was introduced, so to ensure everything continues to work as expected, I re-ran the range test locally.

While debugging, I found that the test was failing in the third scenario where the datetime value falls within the defined range. Due to the newly introduced behavior Here ✨ Datetime fields should utilise #required_error Needs review , the form state now includes additional errors such as datetime_required_error and datetime_no_required_error.

To address this, I updated the third scenario’s assertion to only check that range_datetime_element is not present in the form state errors, as this is the specific error we are handling in this case in testRangeValidate() test method. With this change, the test now passes as expected.

Moving this back to Needs Review.Kindly review.

For me the MR did not apply on 11.1.5.

I had a similiar issue that if the source file is different on the translation than the name would not be updated and always gets the new source file title.

My WA would be to check on the $_POST variable but that feels like a real ugly hack.

if ($this->activeLangcode === $langcode && $_POST['name'][0]['value'] ?? '' !== $translation->getName()) {
        $translation->setName($_POST['name'][0]['value']);
      }

Changed the title, Please review.

Working on it.

This still needs a title that will have meaning when scanning the git log years from now. It is changing the API and adding a new method, after all. Something along the lines of 'Add BreakpointInterface::hasMediaQuery() for ...'?

Re rolled Patch #53 for drupal 11.1.6

Automatically closed - issue fixed for 2 weeks with no activity.

Consensus seems no test then change seems straight forward

I found that there is work going on elsewhere for this, including:

• Another contrib module, that has also implemented a modal approach and has more installs than this module: https://www.drupal.org/project/media_library_edit →
• Core design issue: 📌 Design a UI to allow various kinds of alterations to referenced Media entities in a modal Active
• Core implementation issue: ✨ Allow media items to be edited in a modal when using the field widget Postponed

Added test coverage as well. Existing phpstan/d12 depreciations should remain out of scope.

Would be great if someone can review so we can get this fairly long standing one in.

Thanks!

Okay sorted for 2.1.x. Tests pass again BUT this could use more test coverage for the new functionality.

I also implemented a variation of @jrockowitz suggestion to improve the UX with checkboxes rather than select multiple.

For those updating to this patch, since update hooks are not done between patch versions, the simplest is to:

1. Control + F your config/sync/ directory for 'editable_tags'
2. Nest them within the tag group ID like this diff below:

 settings:
   editable_tags:
-    title: title
-    description: description
+    basic:
+      title: title
+      description: description

nod_ → closed merge request !7792

Committed 23301df and pushed to 11.x. Thanks!

✨ Support CKEditor5 Needs review has been closed and marked fixed. Since this now has a working solution in contrib, should this issue re-focus efforts around inclusion of the functionality from https://www.drupal.org/project/edit_media_modal → into core's Media Library CKE5 integration?

scott_euser → changed the visibility of the branch 3108108-allow-which-metatags to hidden.

scott_euser → changed the visibility of the branch 2.1.x to hidden.

Hmmm can't seem to get the fork to sync from upstream repo, not sure if its because I'm not the owner of the fork. No possibility to create a new fork from 2.1.x. I could create a new issue, but shame to lose all the history

scott_euser → changed the visibility of the branch 8.x-1.x to hidden.

scott_euser → changed the visibility of the branch 3108108-allow-which-metatags to active.

scott_euser → changed the visibility of the branch metatag-3108108 to hidden.

Bot changed status, but I think it is ready for review?

Ah right, I'd forgotten that the textarea widget isn't used on text field items. Maybe the unit test is enough, so putting to RTBC.

OK, so no tests.

However, with that 'typo' fixed the script fails to run.Doing so results in the error reported by @jaydev bhatt, in #9. The solution in the MR is too bootstrap Drupal so that t() is found. That seems unnecessary since this isn't doing any translations. So, I did some digging.

$ git log -- core/scripts/update-countries.sh reported 🐛 Remove redefintion of t() from update-countries.sh Fixed was committed in Sep 2023. Reading that issue brought back memories of being here before. Now that there is an argument in the t() functions in CountryManager I think the fake t() removed in #3384436 can be restored.

Apologies; I had to read up more on Drupal etiquette about changing status - I've used the patch from #38 extensively without issue.

@solideogloria

Would this also make it so that files will be marked temporary when the Entity Usage module says that there are no more uses? That's what I think should happen if the site has $config['file.settings']['make_unused_managed_files_temporary'] = TRUE;

Not currently, but it could theretically be easily added as a File entity pre-delete hook or equivalent, that does the appropriate checks for it.

However given how it's a major change in behaviour from how it previously worked, I'd recommend we add this functionality as a follow-up or separate issue hidden behind a configuration or something since the accidental loss of data is something we'd want to avoid where possible.

Note: this means that XB will start refusing ViewsBlocks whose Views use arguments, because:

          // Look for arguments and expose them as context.
          foreach ($display->getHandlers('argument') as $argument_name => $argument) {
            /** @var \Drupal\views\Plugin\views\argument\ArgumentPluginBase $argument */
            if ($context_definition = $argument->getContextDefinition()) {
              $this->derivatives[$delta]['context_definitions'][$argument_name] = $context_definition;
            }
          }

— \Drupal\views\Plugin\Derivative\ViewsBlock::getDerivativeDefinitions()

And that's a good thing, because those blocks would not work as expected in XB today, because 📌 Handle block contexts Active is not implemented yet. 👍

Thanks, this looks superb!

A refreshingly easy test to update. Those are a bit hard to find in XB 🙈

That's literally what we're working to improve → .

This InvalidArgumentException is also being triggered by bot traffic on all our sites using the Lazy module because it uses this RequestPath condition plugin. It's being triggered by visiting the path /index.php.suspected, so not just paths with double slashes. That's why I want to propose to also just catch any InvalidArgumentExceptions thrown in the getAliasByPath() method.

Automatically closed - issue fixed for 2 weeks with no activity.

Automatically closed because Drupal 7 security and bugfix support has ended → as of 5 January 2025. If the issue verifiably applies → to later versions, please reopen with details and update the version.

Automatically closed because Drupal 7 security and bugfix support has ended → as of 5 January 2025. If the issue verifiably applies → to later versions, please reopen with details and update the version.

@godotislate this issue is about textarea elements. It looks like core's fields with textarea elements don't have settings for maxlength attributes. Am I missing something?

I'm successfully using the merge request in #38 on a sync from an external MySQL data source with no issues.

Hi, wondering about this in my project. If I understand correctly, if a transition stays the same, and the entity exists (edit form, not create form) - no email is sent.

Example: content type "Job Reference" has reference email field, when the reference is created (submitted to submitted transition) email template is fired of to the reference person to complete the form.

However, job applicant realizes email address was bad, and needs to correct that, hits the edit tabs and fixes the email address. Clicks Save, (submitted to submitted transition still) and this time, no email is sent to the corrected email address.

Well, this is my experience. I get how this would not work for other use cases, would be nice to have a global configuration :

Send email template(s) to same state transitions (submitted to submitted) Y/N ?

A refreshingly easy test to update. Those are a bit hard to find in XB 🙈

A review of the current code is needed before moving forward with this. After that, we can go ahead with writing tests for this.

1. Type "modal" worked fine.
2. Fixed issues for type "off_canvas"
3. Handled for the "off_canvas_top"

To quickly test, return the following code in a block or controller:

Open link in a "modal" window:

    $url = Url::fromRoute('entity.node.canonical', ['node' => 1], ['absolute' => TRUE]);
    $link = Link::fromTextAndUrl($this->t('Read more'), $url);
    $link->openInRenderer('modal');
  return [
      'read_more' => $link->toRenderable(),
  ];

Open link in an "off-canvas" window:

    $link->openInRenderer('off_canvas');

Open link in an "off_canvas_top" window:

    $link->openInRenderer('off_canvas', ['position' => 'top']);

This would be great to get done, so we don't need to rely on layout_builder_st which has been lagging for D11 support.

Assigning myself to review questions raised in #70 & #72...

Have just created an MR from the patch submitted in #18 → . Going to test these methods locally first.

prashant.c → changed the visibility of the branch 2944554-allow-easily-creating to hidden.

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

Hi @everyone,
I am using drupal 10.4.4 and still facing the paging issue when it loads the url than it shows the default 48 results and when i click on page 2 than i can see correct page result is coming for me, and when again going to first page number than it is showing the correct result, what ever is set in full pager count, and ajax is on for that views,
also I have tried the patch to apply, but no patch works for me.
please suggest.

Was trying to find occurences of

$url->setOption('query', \Drupal::destination()->getAsArray())

and $url->setOption('query', ['destination' => 'other/path'), could only find one, pushed that change.

Tests were added, so I am removing the tag for now.

By default XB adds a 'None' value to the SELECT OPTIONS, which is great.
But it doesn't seem possible to set this None value as the default value.

What I tried:
- Not to add examples at all
- examples: []
- examples: [NULL]
- examples: ['']

All of them return the same error
Twig\Error\RuntimeError: An exception has been thrown during the rendering of a template ("[linno_theme:select/select] Does not have a value in the enumeration ["ratio-32x9","ratio-21x9"]. The provided value is: ""."). in Twig\Template->yield() (line 1 of themes/custom/linno_theme/components/select/select.twig).

Confirming #43 Still works on 10.4.6

Using layout builder, paragraphs, content moderation & entity reference revisions.

@deviantintegral joined me in a pairing session to step-debug the test failures.

andy-blum → credited deviantintegral → .

Don't have to convert all but a few help show usefulness

I just did a keyword search in the codebase, and a few files in which this could be used are:

• https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/block...
• https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...
• https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...
• https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/conte...

There could be many more like these.

Generally I'd argue that tests for specific functionality that a module provides should live in that module. We have however broken that pattern at the beginning with the general test and later on with the views integration.

For general tests:
- https://git.drupalcode.org/project/lupus_decoupled/-/blob/1.x/tests/src/...
- https://git.drupalcode.org/project/lupus_decoupled/-/blob/1.x/tests/src/...
I think it is fine if they stay on the top level module, but the views tests should go into the lupus_decoupled_views submodule.

Because of the browser behavior described in #19, I think it makes sense to include a Browser test after all. I think it'd also make sense to test that the saved value(s) do not exceed DB column sizes, so I think the test could look like this:

• Create an content entity type with a text field (Drupal\text\Plugin\Field\FieldType\TextItem) and set the maxlength storage setting to something low
• Visit entity create form and submit the form where the text field value length should equal the maxlength. This actually should be done three times, with "\r\n", "\n", and "\r" separately included in the text value
• Save should occur without a database exception
• Load the entity from database storage after save and confirm that the text contains "\n" and not "\r\n" or "\r"

MR threads addressed ... so back to review.

Test coverage is good, but I mean where in core could this be needed? Typically I see that functions that aren't called anywhere in core get deprecated and removed. So think we need least a spot or two in core where this is needed.

📌 Calculate field and component dependencies on save and store them in an easy to retrieve format Active is making this a lot simpler.

I have added re-roll patch of the patch in #18 for 11.x version

• Rebased the merge request and resolved minor PHP CodeSniffer issues.
• The current permission name, "View own account details", is too generic for its actual purpose.
• This permission specifically controls access to the roles section on the user account edit page.
• Even without this permission, users can still view and edit their own account details.
• Ideally, if we retain the current permission name, it should only apply to the "/user/[uid]" page, and not to "/user/[uid]/edit".
• In my opinion, for better clarity and purpose alignment, a more appropriate permission name would be something like "View user roles" or "View own roles".

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

@mayur-sose So what was it related to?

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Hello,
Verified and Tested on Drupal 11,
MR applied successfully,
Working as expected

Steps to reproduce
1. Install Drupal 10.1.x-dev version.
2. Go to "Configuration"->Basic Site Setting-> Add Site Slogan->Save.
3. Go to Homepage and Click on the Site branding "Configure Block option and enable site slogan checkbox -> Save Block
4. Go to Homepage and observe the slogan not displayed in the site branding block.

Check attachments -
Moving this to RTBC

Seems to have pipeline failures. May be worth converting a spot where this is needed to help show why it should be added.

Thank you for sharing your idea for improving Drupal.

We are working to decide if this proposal meets the Criteria for evaluating proposed changes. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or there is no community support. Your thoughts on this will allow a decision to be made.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!

Thank you for creating this issue to improve Drupal.

We are working to decide if this task is still relevant to a currently supported version of Drupal. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or is no longer relevant. Your thoughts on this will allow a decision to be made.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!

Fyi: 8.4 comes with native \DateTime::createFromTimestamp, which allows int|float. https://github.com/php/php-src/commit/88f2dc626862b4c40ea20b8029838a8d0d....

$timestamp = 2147483647/1.1;

// Origin float
var_dump($timestamp);

$timestamp_casted = (int) $timestamp;

// Casted as int
var_dump($timestamp_casted);

// Output with float
var_dump(DateTime::createFromTimestamp($timestamp));

// Output with casted float
var_dump(DateTime::createFromTimestamp($timestamp_casted));

Output in 8.4.6:

float(1952257860.9090908)
int(1952257860)
object(DateTime)#1 (3) {
  ["date"]=>
  string(26) "2031-11-12 13:51:00.909091"
  ["timezone_type"]=>
  int(1)
  ["timezone"]=>
  string(6) "+00:00"
}
object(DateTime)#1 (3) {
  ["date"]=>
  string(26) "2031-11-12 13:51:00.000000"
  ["timezone_type"]=>
  int(1)
  ["timezone"]=>
  string(6) "+00:00"
}

So the best is to follow php-core.

I've created a MR that applies the patch in #26 (after checking: issue still exists, patch still applies, patch fixes the issue).

Can anybody please confirm that this needs test coverage? I cannot find any existing tests that verify if the expand functionality works for normal links, so it seems a bit odd to write a test only for <nolink> items. Thanks!

Was looking for the generic methods for the same. I have raised an MR with some additional checks, like the path should be internal only. Once this is reviewed, we can go ahead with writing tests also.

Current changes need to be reviewed

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

I came across this comment https://www.drupal.org/project/drupal/issues/3028868#comment-13812371 → , which highlights that the Drupal image module does not support SVGs for a variety of reasons. Given this, it is unclear whether the current issue is still relevant.

Consequently, it appears that utilizing the SVG Image module might be a more suitable alternative, as it offers a widget and formatter for handling SVG fields.

I'm daring to self-RTBC this because:

• The update was requested by a core committer who said the MR would probably be ready afterward.
• The update removed somewhat arbitrary code style changes in the MR.
• The update simplified the diff.

But since there are a lot of commits that have happened since #138 I figure that a good, old-fashioned interdiff wouldn't go amiss.

Agreed with #10. Also if we'd noticed this quickly we might have reverted the original issue and we don't add new year coverage with a revert.

smustgrave → closed merge request !2964

Recent commits trying to achieve #138. I think need to add back in 2 of the params following the same typehint-no-constructor-property-promotion pattern. Also need to figure out which params to put in parent and which to put in the current class constructors.

I think that this probably does not need tests, as it is a simple typo from a previous issue (see this commit). Checking by https://www.drupal.org/about/core/policies/core-change-policies/core-gat... → :

1. The issue has clear ‘Steps to reproduce’ in the Issue Summary. - YES
2. The fix is 'trivial' with small, easy to understand changes. - YES

Questions
1. Is the fix is easy to verify by manual testing? - YES
2. Is the fix in self-contained/@internal code where we expect minimal interaction with contrib? Examples are plugins, controllers etc. - YES
3. Is the fix achieved without adding new, untested, code paths? - YES
6. If this fix is committed without test coverage but then later regresses, is the impact likely to be minimal or at least no worse than leaving the bug unfixed? - YES

What do you think?

Apologize for taking so long to get back to this one, even after posting it in slack lol

But feedback appears to be addressed.

Can the issue summary be cleaned up some. Using some hybrid template (maybe that was the template back then haha) but updating it would help with reviews.

Believe all views in core have been addressed, double checked that views.view.front in node actually didn't have any format use also (just to follow up on #22)

1) Drupal\Tests\migrate\Unit\MigrateSourceTest::testPreRollback
Expectation failed for method name is "set" when invoked 1 time.
Method was expected to be called 1 time, actually called 0 times.
FAILURES!
Tests: 13, Assertions: 37, Failures: 1.
Exiting with EXIT_CODE=1

Shows the test coverage is there.

Migration is not my best but going off @quietone in #16 mentioning this is still valid as a migration maintainer.
Solution matches the MR

Believe this one is good, but if wrong I apologize.

Believe feedback has been addressed on this one.

Apparently PHP blows up with a fatal error if they're typehinted. See the results of https://git.drupalcode.org/issue/drupal-1578832/-/pipelines/476013. I don't really get why.

Oh I get it now - CacheCollector doesn't type hint the class, so they can't be constructor property promoted and also type hinted.

In this case what we need to do is remove the constructor property promotion, and add back the type hints.

Needs work for that but I think this is ready otherwise.