- 🇩🇰Denmark ressa Copenhagen
To prevent this issue from stalling, it would be amazing if someone with the right skills could check locally with Composer if the scaffolding in the patch:
- Creates a
.gitignore
- Which is not overwritten/updated
- Creates a
Automatically closed - issue fixed for 2 weeks with no activity.
Automatically closed - issue fixed for 2 weeks with no activity.
Automatically closed - issue fixed for 2 weeks with no activity.
Automatically closed - issue fixed for 2 weeks with no activity.
- 🇬🇧United Kingdom longwave UK
The problem is that by leaving the method on the interface, even if deprecated, is that anyone else implementing has to also leave the implementation in place. Instead I have still removed the method from the interface, but kept it on the concrete class, and added BC so it still works but issues a deprecation if you call the old method.
This is also too late for 10.3 now, so I have deprecated in 11.1 for removal in 12.0.
- 🇨🇦Canada mgifford Ottawa, Ontario
Thanks @rkoller this is great. I'd like to see this patch include Ralf's suggestion to collapse the first drop button as soon as another drop button is expanded. We don't want to introduce an accessibility issue for keyboard only users, by fixing an issue for screen reader users.
I would say that from Ralf's demo that the patch would currently violate SC 2.4.11: Focus Not Obscured (Minimum) (Level AA).
Let's open up a follow-up item about the drop button label, as suggested by Ralf.
- 🇩🇪Germany rkoller Nürnberg, Germany
I've tested MR5231 and created a short video (see dropbutton.mp4 - recorded on macOS 14.5 and safari 17.5 with voiceover). In general the first point of the proposed solution looks good, the open and closed state are commenunicated, that's perfect.
In regards of the second point reducing the drop button label from "list additional actions" to just "additional actions" is a good choice, i agree to that. The only thing i wonder is if it would make sense to append the context the button is in, front load "additional actions" and then append the context. that way a screenreader user could jump off at any time but still has the option or in case the person has a small working memory to reassure it is the correct button in the correct context (also if someone navigates by the rotor). the
edit
button as well as thedelete
option already provide that context while theadditional actions
button andtranslate
option do not? I would consider it out of the scope for this issue but moving "providing a context" into a follow up might be a reasonable step, what do you think?In regards of the third point, keeping the dropdown up when focus and hover are out i consider a good thing but as soon as another drop button is expanded it looks and feels odd (check dropdown.mp4)? wouldnt it make sense to collapse a drop button as soon as another is expanded? would be the same pattern that was used for the subnav of top level menu items in the admin_toolbar?
and @camilledavis you've suggested to allow the user to collapse the drop button by pressing ESC in #4 ✨ Dropbutton widget not accessible to screen readers Needs review . I would +1 that option.
- 🇩🇪Germany rkoller Nürnberg, Germany
We discussed this issue at 📌 Drupal Usability Meeting 2024-05-17 Needs work . That issue will have a link to a recording of the meeting.
For the record, the attendees at the usability meeting were @AaronMchale, @benjifisher, @rkoller, @shaal, @skaught, and @simhohell.
If you want more feedback from the usability team, a good way to reach out is in the #ux channel in Slack.
While exploring the new responsive image form display widget we ran into one problem. On a standard 11.x-dev install with the responsive image module installed, and MR4628 applied, we had the following three options available for
Preview responsive image styles
:- Narrow
- Wide
Our assumption was that based on micro copy you would either get the preview image added in the dimensions of the chosen responsive image style or you have no preview depending on your choice. But problem was no matter what setting we set the resulting preview looked like this:
Our expectation, based on the micro copy, would have been that this would only be the result in case option 1, no preview, was chosen. So we were not sure if this is a bug, and in general we've asked us how this feature is intended to work? We would need some clarification in that regard before we are able to make any recommendations.
We will also continue our initial discussion we've started in regards of the micro copy. There was already a clear consensus to change the label
Preview responsive image style
toResponsive image style for preview
. "Technically" a user isn't "previewing a responsive image style", instead a preview image based on that responsive image style is being generated. About the corresponding description, there was the consensus to clarify where the image is shown (aka that it is shown on the edit form), and that "preview responsive image" is too verbose as well as misleading (There is only a single image being used, based on the responsive image style - but again we need to know how the feature is supposed to work).
One radio button option is calledBar with progress meter
while within the description it is referred to as aprogress bar
. Aside this inconsistency in terminology it was also noted that the first sentence of the description is imprecise, the throbber actually is communicating the status of the upload, it is telling the user if the upload is still underway or was already finished. But the microcopy about the progress indicator would apply to regular image widgets as well. So making adjustments in that regard would be more suitable for a followup issue to keep things consistent between image and responsive image widgets.But we will finish the discussions about the micro copy as well as the functionality in general as soon as the feedback is in. Thank you in advance!
Not sure what the best status would be in this case? I went with needs work but to postpone with maintainer needs more infos felt wrong since i am not the maintainer nor person working on this issue. I'll go with needs work, but please adjust accordingly if that is not the right choice and there is a more suitable one. Apologies in that case.
- 🇺🇸United States smustgrave
Okay so think this doesn't need an issue summary actually.
Tabbing through the hover is now activated
LGTM
- 🇮🇳India Mithun S Bangalore
I have validated before MR that the slogan is not displaying on the UI for olivero theme.
Validated by switching to the branch and verified the slogan is visible/hidden as per the users choice and based on the block configuration. Attaching the screenshots for reference.RTBC +1.
- achap 🇦🇺
I've converted those annotations to php attributes. Unfortunately when I've merged in the latest code from 11.x there is a test failure in
testRowSkippedEvent
however the error message is obscured by the annoying "Exception: Serialization of 'Closure' is not allowed" message which doesn't actually show what's wrong. When I run MigrateEventsTest.php locally I don't get any errors so this one is hard to debug. Apparently this is quite a common issue faced in Migrate tests (see related issue).I do have a stack trace:
/builds/issue/drupal-2756269/core/lib/Drupal/Core/Cache/MemoryBackend.php:119 /builds/issue/drupal-2756269/core/lib/Drupal/Core/Cache/CacheCollector.php:275 /builds/issue/drupal-2756269/core/lib/Drupal/Core/State/State.php:102 /builds/issue/drupal-2756269/core/modules/migrate/tests/src/Kernel/MigrateEventsTest.php:481 /builds/issue/drupal-2756269/vendor/symfony/event-dispatcher/EventDispatcher.php:206 /builds/issue/drupal-2756269/vendor/symfony/event-dispatcher/EventDispatcher.php:56 /builds/issue/drupal-2756269/core/modules/migrate/src/MigrateExecutable.php:247 /builds/issue/drupal-2756269/core/modules/migrate/tests/src/Kernel/MigrateEventsTest.php:354
That shows that it is trying to serialize the cached data (which would contain an exception). My guess would be that the exception data has a closure in there somewhere but like I said very hard to debug if I can't reproduce it locally.
Hopefully someone can help pinpoint the issue!
- 🇷🇸Serbia DavorHorvacki
Updating NULL values to either 'a:0:{}' (map field) or 'N;' (almost any other) on problematic column(s) once also resolves problem.
No patch needed. You can create update hook for this.This works because problem is appearing only when you are adding field to entities with existing entries.
- 🇮🇳India Mithun S Bangalore
Added a rebase and resolved the merge conflicts. Please review.
- 🇮🇳India Mithun S Bangalore
Mithun S → made their first commit to this issue’s fork.
- 🇳🇿New Zealand DanielVeza Brisbane, AU
I was concerned the new approach could stop the message from showing when we do want it to display, but I've checked all child classes of
EntityDisplayFormBase
and the only one that sets redirects in the form state isLayoutBuilderEntityViewDisplayForm
.I prefer the approach taken here to my original MR & it's flexible enough to be used in the future if something like this pops up again. So +1 from me
Closing as duplicate of https://www.drupal.org/project/drupal/issues/3360837 🐛 Users with permissions for toolbar but not administration tray get "Uncaught TypeError: $orientationToggleButton[0] is undefined" in console Needs work
Added a draft change record.
Just tested with Gin, it doesn't seem to be affected due to not using Claro's page.html.twig
- @camilledavis opened merge request.
- First commit to issue fork.
- 🇺🇸United States smustgrave
Seems new approach still has appropriate coverage
1) Drupal\Tests\media\Unit\ResourceFetcherTest::testFetchTimeout TypeError: Double\GuzzleHttp\Client\P1::request(): Return value must be of type Psr\Http\Message\ResponseInterface, null returned /builds/issue/drupal-3228350/core/modules/media/src/OEmbed/ResourceFetcher.php:67 /builds/issue/drupal-3228350/core/modules/media/tests/src/Unit/ResourceFetcherTest.php:52 ERRORS! Tests: 2, Assertions: 5, Errors: 1.
believe the change is good and title makes sense.
Tweaked versions slightly but assume that will have to be updated if it makes it in 10.3 or 10.4
- 🇺🇸United States smustgrave
I'd be lying if I said I fully understand the view config update (not one of the 5 haha) but running the test-only feature the tests for the update are failing as expected
1) Drupal\Tests\views\Functional\Update\EntityArgumentUpdateTest::testViewsFieldPluginConversion Failed asserting that two strings are equal. --- Expected +++ Actual @@ @@ -'entity_target_id' +'numeric'
Can see the full list of expected test failures here https://git.drupalcode.org/issue/drupal-2640994/-/jobs/1604882
So believe the addition of that is good
- 🇬🇧United Kingdom longwave UK
Thanks! It's even easier to override than that, you can just add a
services.yml
alongside yoursettings.php
(and include it fromsettings.php
), and override any container parameter there - updated the IS a bit for this. - 🇺🇸United States amanire
Good to see progress here! FWIW I just deployed the patch in #48 to a production site and it resolved the deprecation warnings described in https://www.drupal.org/project/profile/issues/3324465 📌 [PHP 8.1] Deprecated function: unserialize(): Passing null to parameter #1 ($data) of type string is deprecated in Drupal\Core\Entity\Sql\SqlContentEntityStorage->mapFromStorageRecords() Closed: duplicate . I was a little surprised at the substantial CPU performance improvement for the site, presumably because these were invalidating the varnish cache.
- 🇬🇧United Kingdom scott_euser
Sorry missed saving my title change clearly - whoops!
- 🇬🇧United Kingdom scott_euser
Thanks for flagging, good point! I have updated both, attempting to match the format of #3202145: oEmbed resource fetcher needs to set a reasonable connection timeout → for the title.
My link to the change record was wrong (I had linked to this issue!), here is the change record https://www.drupal.org/node/3362722 → .
- 🇺🇸United States bradjones1 Digital Nomad Life
Marking this NR - I haven't chased my tail like this in quite a while but I think this ended up in a decent place with a minimum amount of change to logic. Helps reveal where we were depending on cloned properties rather than being explicit.
- 🇳🇿New Zealand quietone New Zealand
In 🐛 Adding media library openers use autoconfigure and tags in 10.3.x has BC consequences Active alexpott noted that this needs a change record.
- 🇺🇸United States dww
Wow, what a colossal PITA. 😆 Guess it's good I jumped through all those hoops, since:
- There were indeed some test views with stale argument plugin definitions that needed to be manually fixed, including the view for testing this new functionality. 😂
EntityArgumentUpdateTest
was making some slightly bogus assertions.
However, I had to completely rewrite
ViewsConfigUpdater::processEntityArgumentUpdate()
to operate on entire View entities, instead of using all the per-handler plumbing we've got. To my great dismay, if you copy the existing patterns,ViewsConfigUpdater
process functions make a bunch of changes to handler config, then try to save the view, but that instantiates anotherViewsConfigUpdater
object (this time, with deprecations enabled) to do all the work again. 🤯 So apparently, The Right Way™️ to update a view with all this plumbing is that your process function has to directly update the$view
(which isn't even normally passed in to your process function if you think you're just dealing with handlers), not just fix the$handler
. 🤦♂️I guess I'm now in the exclusive club of ~5 people on Earth who understand how this class works. 😬 😂
ANYWAY, we're back to green pipelines on 11.x and 10.3.x. I've given up on the 10.2.x MR at this point. It works great for the project where I needed this fixed, all the additional hassles are irrelevant to that site, and this is clearly not getting backported. I just hope this can still land in 10.3.x!
Thanks,
-Derek - 🇺🇸United States mark_fullmer Tucson
This has been rebased of the last commit on 11.x. Setting back to "Needs Review."
- 🇬🇧United Kingdom longwave UK
The issue title and summary no longer match the changes made in the MR following the discoveries in this issue, they need updating to help reviewers understand the changes that are being made and why.
Voiceover announces the row (see screenshot), so I removed the reference to screen readers from the description.
Got my POC work in MR 8070 to go green. So along with the steps taken in #44 📌 Convert MigrateSource plugin discovery to attributes Postponed , the next thing I did was eliminate the idea of multiple and automated providers altogether in plugin attribute-based discovery and introduce the idea of manually defined dependencies instead. Complete summary looks something like this:
- Use
PhpToken::tokenize()
on the plugin class file - Loop through the tokens to find the class declaration token
- Copy all the class file contents before the class declaration to a string
- Replace usage of "self" or "static" with the original class name in the string
- Append a random suffix to the class name for uniqueness, and add a class declaration statement and
{}
to the string - Copy the string to a temporary file and include that file
- Instantiate ReflectionClass on the new temporary class
- Extract attribute data from reflection
- Add a "Dependencies" attribute class that has a "modules" array property which is used to defined other modules the plugin class depends on
- When parsing the class, look for the dependencies attribute first. If it exists, check to make sure all dependencies are installed, and if any dependencies are missing, prevent the plugin info from being saved to file cache
- Update the default plugin manager to check that all dependencies are installed in findDefinitions()
- Add the Dependencies attribute to all migrate source plugin classes that have an implicit dependency on migrate_drupal (or other modules)
This does require developers who write plugin classes to manually account for any implicit dependencies by listing them with the attribute, instead of having discovery determine the providers automatically. While this can be bit of a hassle, I don't think this is a major obstacle, because usually you can determine what dependencies need to be declared by checking the use statements.
This is still POC-ish, but putting back into Needs Review to get feedback on whether this is a good approach. If approach looks good, it might make sense to split out the core discovery part of the work to either #2786355: Move multiple provider plugin work from migrate into core's plugin system → or 📌 Investigate possibilities to parse attributes without reflection Active first before continuing here. But it was useful to test first with all the converted Migrate Source plugins here. Beyond that, there are probably some tests that need to be written for the revised discovery as well.
- Use
- 🇳🇿New Zealand RoSk0 Wellington
Started new merge request targeting 11.x with:
- patch from #48
- suggestion from #54
- and links to #2883851: Add initial values support for configurable and multi-valued fields → from #36
- 🇺🇸United States dww
I got a bit more clarity from @catch on what's expected here. I pushed some commits to the 11.x and 10.3.x MRs to trigger deprecations when
ViewsConfigUpdater::processEntityArgumentUpdate()
changes a view outside of post_update. I'm not sure on the wording of the deprecation message, that could use another pair of eyes. Also unclear if we "need" tests that this deprecation plumbing works as expected. I sure hope that's not my responsibility here. 😅I'll keep an eye on the pipelines to make sure I didn't break anything, but I hope this is now actually RTBC.
- 🇺🇸United States smustgrave
No one has chimed in so maybe it is okay.
1) Drupal\Tests\comment\Functional\CommentPreviewTest::testCommentPreviewOnTranslatedNode Behat\Mink\Exception\ResponseTextException: The text "Preview comment" was not found anywhere in the text of the current page. /builds/issue/drupal-3145146/vendor/behat/mink/src/WebAssert.php:907 /builds/issue/drupal-3145146/vendor/behat/mink/src/WebAssert.php:293 /builds/issue/drupal-3145146/core/tests/Drupal/Tests/WebAssert.php:975 /builds/issue/drupal-3145146/core/modules/comment/tests/src/Functional/CommentPreviewTest.php:287 /builds/issue/drupal-3145146/vendor/phpunit/phpunit/src/Framework/TestResult.php:729 ERRORS! Tests: 4, Assertions: 118, Errors: 1.
Test coverage is there and the fix does solve the issue.
Going to mark.
- 🇨🇭Switzerland Berdir Switzerland
I think it would make more sense if that's implemented in the frontend *before* saving, otherwise you have to go back after you already saved and edit it again. Similar to the you have unsaved changes message after reordering.
That said, opinionated things are tricky to do in core. first, it should be in menu_ui, because node module should not depend on menu link stuff, then it needs to handle cases when the node title field isn't displayed and so on.
- 🇺🇸United States smustgrave
Change record looks good so removing that tag.
Added release note to the summary
But MR appears to need a manual rebase.
- 🇬🇧United Kingdom scott_euser
Thanks, makes sense to me.
Probably just this small tweak to the unit test is fine given that parameters are a Symfony functionality that are already well used, so this will just ensure that that parameter is maintained in the future.
I did update the existing constructor to use promoted constructor arguments to match while in there, but happy to roll that back if you prefer.
In terms of test failure, not sure how core/modules/block/tests/src/Functional/BlockCacheTest.php is related? Perhaps its an issue from somewhere else?
- 🇺🇸United States Kasey_MK
Sorry made the patch from Drupal default branch this time and this one applies against Drupal 10.2.6.
The change that made my webform submissions work again was adding
(count($this->recursionKeys) !== 1)
to line 569 in core/lib/Drupal/Core/Entity/EntityViewBuilder.php but starting from the default Drupal branch added some other changes to the patch. Interdiff against the patch in #155 attached. - @scott_euser opened merge request.
- 🇬🇧United Kingdom scott_euser
scott_euser → changed the visibility of the branch 11.x to hidden.
- 🇬🇧United Kingdom scott_euser
scott_euser → changed the visibility of the branch 3228350-timeout-setting to hidden.
- 🇺🇸United States Kasey_MK
Adding a check that
count($this->recursionKeys) !== 1
allows webform submissions to render without logging an error, but I don't pretend to understand this issue well enough to know what else that might do. Attaching the patch I made and the interdiff from the patch in #155 for review. - 🇺🇸United States bradjones1 Digital Nomad Life
OK, a few hours of my life gone into the depths of the meaning of "original" in various contexts around Drupal.
The genesis of this issue appears to be getting away from a dynamic property, but it's morphed into a larger problem space occupied by related issues such as 🐛 Impossible to update an entity revision if the field value you are updating matches the default revision. Fixed . There's also a heavy dose of translations weirdness here.
Overall, the patch is pretty simple in that it replaces gets and sets of $entity->original with new getOriginal/setOriginal methods accessing what will soon be a protected property of the entity. Simple enough to understand that.
Even though that feels like it should go very smoothly with no regressions, test failures cropped up in code that must use the now-deprecated original property to do all sorts of magic like deciding when a translation has changed. I'm not entirely sure what broke this in every instance but I think it's related to the particulars of cloning the "parent" entity into the translations array.
This feels overall like the right cleanup to do, but the collateral damage is gnarly (as mentioned way back in #5. As you can see from the recent commit history, I went back and forth on various hacks and had myself questioning "how does any of this even work right now?" but we're down to a few failing tests which is pretty manageable.
One failing test in particular has an ominous link to a recently-closed issue relating to the behavior of the original property with revisions. @Berdir mentioned at #2859042-54: Impossible to update an entity revision if the field value you are updating matches the default revision. → that the method introduced in this issue is the correct path forward in smoothing out these special cases. So let's do that?
This one is pretty gnarly but I think this is a move in the right direction and I'm happy to see mostly green with a limited amount of hack.
I tried a POC on an alternate approach for attribute discovery in MR 8070.
The idea is that since only the attributes are needed, parsing can be done like this:
- Use
PhpToken::tokenize()
on the plugin class file - Loop through the tokens to find the class declaration token
- Copy all the class file contents before the class declaration to a string
- Append a random suffix to the class name for uniqueness, and add a class declaration statement and
{}
to the string - Copy the string to a temporary file and include that file
- Instantiate ReflectionClass on the new temporary class
- Extract attribute data from reflection
So basically, core/modules/block_content/src/Plugin/migrate/source/d6/BoxTranslation.php:
<?php namespace Drupal\block_content\Plugin\migrate\source\d6; use Drupal\block_content\Plugin\migrate\source\d7\BlockCustomTranslation as D7BlockCustomTranslation; use Drupal\migrate\Attribute\MigrateSource; /** * Drupal 6 i18n content block translations source from database. * * For available configuration keys, refer to the parent classes. * * @see \Drupal\migrate\Plugin\migrate\source\SqlBase * @see \Drupal\migrate\Plugin\migrate\source\SourcePluginBase */ #[MigrateSource( id: 'd6_box_translation', source_module: 'i18nblocks', )] class BoxTranslation extends D7BlockCustomTranslation { /** * Drupal 6 table names. */ const CUSTOM_BLOCK_TABLE = 'boxes'; const I18N_STRING_TABLE = 'i18n_strings'; }
gets copied to /tmp/BoxTranslationadfrghq.php with these contents:
<?php namespace Drupal\block_content\Plugin\migrate\source\d6; use Drupal\block_content\Plugin\migrate\source\d7\BlockCustomTranslation as D7BlockCustomTranslation; use Drupal\migrate\Attribute\MigrateSource; /** * Drupal 6 i18n content block translations source from database. * * For available configuration keys, refer to the parent classes. * * @see \Drupal\migrate\Plugin\migrate\source\SqlBase * @see \Drupal\migrate\Plugin\migrate\source\SourcePluginBase */ #[MigrateSource( id: 'd6_box_translation', source_module: 'i18nblocks', )] class BoxTranslationhhgsagp {}
and reflection is instantiated on
Drupal\block_content\Plugin\migrate\source\d6\BoxTranslationhhgsagp
, so the attributes can be read, without concern of inheriting from any unknown class, interface, or trait.This seemed to work OK with some limited local testing, but CI caught a significant error (among some other failures I haven't investigated). For a class like core/modules/system/tests/modules/cron_queue_test/src/Plugin/QueueWorker/CronQueueTestSuspendQueue.php:
<?php namespace Drupal\cron_queue_test\Plugin\QueueWorker; use Drupal\Core\Queue\Attribute\QueueWorker; use Drupal\Core\Queue\QueueWorkerBase; use Drupal\Core\Queue\SuspendQueueException; use Drupal\Core\StringTranslation\TranslatableMarkup; /** * A queue worker for testing suspending queue run. */ #[QueueWorker( id: self::PLUGIN_ID, title: new TranslatableMarkup('Suspend queue test'), cron: ['time' => 60] )] class CronQueueTestSuspendQueue extends QueueWorkerBase { /** * The plugin ID. */ public const PLUGIN_ID = 'cron_queue_test_suspend'; /** * {@inheritdoc} */ public function processItem($data) { if ($data === 'suspend') { throw new SuspendQueueException('The queue is broken.'); } // Do nothing otherwise. } }
When the class contents are removed, the constant PLUGIN_ID is undefined, so
self::PLUGIN_ID
in the attribute causes an error. I'm not sure how to get around this, so I think this approach might need to be abandoned. I thought I'd document it here regardless.- Use
godotislate → changed the visibility of the branch 3421014-alternate-attribute-parsing to hidden.
- @godotislate opened merge request.
- 🇺🇸United States dww
I asked in #bugsmash for help on this. @catch pointed me to 📌 ckeditor5 and editor module tests rely on hook_editor_presave() bc layers Active , but that issue has nothing to do with
ViewsConfigUpdater
. I still don't exactly understand what's being asked of me.Views is doing this:
function views_view_presave(ViewEntityInterface $view) { /** @var \Drupal\views\ViewsConfigUpdater $config_updater */ $config_updater = \Drupal::classResolver(ViewsConfigUpdater::class); $config_updater->updateAll($view); }
@alexpott, are you proposing we should change the API for
ViewsConfigUpdater::updateAll()
to take a 2nd argument to determine if we're being called frompresave
orpost_update
? Or do you propose thatviews_view_presave()
checks the return value fromViewsConfigUpdater::updateAll()
and if it ever gets back TRUE it should always trigger a deprecation?Can we pretty please move all such complications to a dedicated follow-up, and fix this before the 10.3.0-beta1 ships? I still fail to see why this poor bug needs to be the one to deal with all that additional functionality.
Tentatively moving back to RTBC to get more committer eyes on this.
Thanks,
-Derek - 🇫🇷France fgm Paris, France
@2dareis2do 's answer on Slack :
My patch is just a collection of other peoples commits so I am not sure I am the best qualified person to review this. This patch has been working for me though. In the meantime I have decided to restrict stats view to registered users as as well.
- 🇺🇸United States Kasey_MK
Confirming @kmonty's report in #163 using patch in #155.
- 🇺🇸United States smustgrave
Rebased and fixed phpcs issue
Hiding patches and old MR (but then un-hid_Will remove tests tag as test branch is showing failure.
But can we update the MR 1594 for 11.x and include tests too please.
- 🇺🇸United States smustgrave
smustgrave → changed the visibility of the branch 2865920-when-a-site to active.
- 🇺🇸United States smustgrave
smustgrave → changed the visibility of the branch 2865920-when-a-site to hidden.
- First commit to issue fork.
- 🇺🇸United States smustgrave
Finally got around to this one
Did a fresh Umami install without the MR applied to verify column shifts on pages like /articles
Did a fresh Umami install with the MR already applied so the config imported
Visited /articles page to verify the layout is the same and responsive at various breakpointsThen I did the same for the homepage as I saw that view was updated.
Also did not notice any issue.Believe this is a good replacement.
- 🇺🇸United States smustgrave
So applied the MR locally.
Update hook ran just fine
I'm still able to update the settings without issue
The proposed schema changes believe make sense.
+1 from me.
- First commit to issue fork.
- 🇮🇳India divya.sejekan
Verified for Drupal - 10.2 . The slogan is visible now.
Verified with Drupal 11.x-dev .The Slogan is visible now
Verified with disabled logo . The slogan is visibleSteps to reproduce
1. Install Drupal.
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.But the patch has white space warning
- 🇺🇸United States smustgrave
So for this one I applied the MR locally.
Looking at the files I went to a view page (Content for this test)
While adding a new field in the popup I did a resize and everything seemed to behave as beforeI enabled layout builder and enabled for Article content type
Adding a section I dragged the off canvas region without issue.Believe this one is good.
- 🇺🇸United States smustgrave
Screenshots appear to have been added and issue summary updated
Believe this is ready for accessibility.
- 🇫🇷France rkcreation
Tested with Drupal 10.2.6, it works well with pager inside modal and without views. I use pager render array like this :
[ '#type' => 'pager', '#route_name' => '<current>', ]
Confirming patch #92 works on PHP 8.1.28 / Drupal 10.2.6.
Unfortunately patch #99 did not work.
- 🇮🇳India arunkumark Coimbatore
Found an issue during the test. If the Logo is unchecked(Show only Sitename and Slogan), none of the details are showing.
- First commit to issue fork.
- 🇨🇭Switzerland idflood
I tried to reroll the merge request for drupal 10.2 but I'm not sure of the correct workflow, especially since the current merge request is based on the 10.1 branch.
Here is the commit I made: https://git.drupalcode.org/issue/drupal-2325899/-/commit/d665b876f6c1ab2...
And I'm attaching a patch which should hopefully work on 10.2.6
- 🇦🇺Australia VladimirAus Brisbane, Australia
VladimirAus → changed the visibility of the branch 3261924-avoid-warning-from-d11.0 to hidden.
- @vladimiraus opened merge request.
- 🇦🇺Australia VladimirAus Brisbane, Australia
Trying to bring all changed into new branch as pull from 11.x returns too many merge conflict. 🤷♀️
Also, changing branch to 11.0.x - 🇮🇳India arunkumark Coimbatore
Reviewed the MR !7792 is resolving the issue. Attached screenshot after applying the MR.
Before applying the patch:
After applying the patch:
Different logo and Long slogan
Ready for RTBC.
- 🇮🇳India sukr_s
without correcting the assertion, the test will fail. do confirm if you want this to be removed. will the merge happen with test failing?
- last update
5 days ago 29,689 pass - 🇺🇸United States mmunjeti
mmunjeti → changed the visibility of the branch 1797438-html5-validation-is to hidden.
- 🇺🇸United States mmunjeti
mmunjeti → changed the visibility of the branch 1797438-html5-validation-is to active.
- 🇺🇸United States ksenzee Seattle area
I worked with my team (@dcmorris, @jmarcella, @tinycanary) at Drupalcon Portland 2024 to resurrect this patch and see what's left of it in 11.x. We'll post results soon.
- 🇺🇸United States bnjmnm Ann Arbor, MI
When I filed this issue in 2020...
- Claro was an experimental theme. It has since become Drupal's admin theme
- I was not a frontend framework manager. I have since become a frontend framework manager
Given that it is 2+ years since Claro graduated from experimental status, I'm not sure the benefits of of moving the assets + references outweigh the potential disruption. Testing this manually requires time and it is a lengthy process without tests to confirm things are OK. Something unwanted could slip through, and custom modules/themes may be referencing some of these assets. Had this happened while Claro was still experimental I'd be all for the change. While it should have occurred when Claro was added to core on October 4, 2019, that doesn't necessarily mean it should happen now. I'm setting this to Closed (outdated), but I don't intend this to be a stern "final word". If anyone here believes the benefits of doing this outweigh the risks then document that and re-open.
Alternatively, get some quick issue credit and open an issue that simply removes this from the readme:
Icons in this folder are copied from Drupal core. This folder with its content should be removed before moving Claro to Drupal core
- 🇺🇸United States bnjmnm Ann Arbor, MI
We are making changes in claro templates only.Do we need to do the same of default theme and then expand tests or shall we create a follow-up issue for default theme?
\Drupal\form_test\Form\FormTestDetailsForm
already tests the default theme. it is simplest to add a few lines to that test to run the test with the default theme and then with Claro. Use a@dataProvider
Set up your dataProvider and put this at the beginning of the test to set the theme when
$theme
isn't false, and that does everything needed without having to add a new test class that is 99% the same as an existing one.if ($theme) { $this->config('system.theme') ->set('default', $theme) ->save(); }
- 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
Thanks for the many reviews, much appreciated.
- 🇬🇧United Kingdom catch
Don't have time to commit this today (or probably this week - about to head afk for at least a couple of days), but +1 from me.
- 🇺🇸United States generalredneck
That's a new file... png based on the diff.
diff --git a/core/tests/fixtures/files/image-test-iccp-profile.png b/core/tests/fixtures/files/image-test-iccp-profile.png new file mode 100644 index 0000000000000000000000000000000000000000..29cb8945f8e4c1ae40de24bf888715e4d870a164 --- /dev/null +++ b/core/tests/fixtures/files/image-test-iccp-profile.png @@ -0,0 +1,24 @@ +‰PNG
- 🇺🇸United States rraney
Just curious about Patch 65. What's all the garbage text at the end of the file?
- 🇺🇸United States nicxvan
Looks good to me too.
It seems that @quietone's concerns about the comments have been addressed as well as they both read clearer.
- 🇺🇸United States smustgrave
Ran the test-only job for the 11.x branch, won't post the entire output but can viewed here https://git.drupalcode.org/issue/drupal-3441222/-/jobs/1584320
Coverage appears to be there.
Reviewed the change record and adequately describes the issue that the new settings is overcoming, least to me.
Deprecations in 10.3 appear good to me as well with removals in the 11.x MR.
Believe this one is good +1 from me.
- 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
Test fails in the D11 version seem completely unrelated, as if HEAD is broken
- 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
Think we have a D11 and D10.3 ready MR now. Changed the order of the optional params so that the deprecation would be nicer for D11 where the optional param was still last.
- @kristiaanvandeneynde opened merge request.
- 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
kristiaanvandeneynde → changed the visibility of the branch 3441222-11-without-deprecations to hidden.
- 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
kristiaanvandeneynde → changed the visibility of the branch 10.3.x to hidden.
Make sure that changes made to the form via AJAX are properly tracked in the form state. In Drupal, if a part of the form is added dynamically (like your biz_wrapper), its values might not be properly included in the form state unless the form structure is adequately managed across callbacks.
//
use Drupal\Core\Ajax\AjaxResponse;
use Drupal\Core\Ajax\ReplaceCommand;
public function accountTypeDropdownCallback(array &$form, FormStateInterface $form_state) {
$form_state->setRebuild(true);
$response = new AjaxResponse();
$response->addCommand(new ReplaceCommand('#biz-wrapper', $form['biz_wrapper']));
return $response;
}- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
WOWWWWWWWWWWWWWWWWWWWW!!!!!!!!!!!!!!!!!
We all owe @kim.pepper thanks, beers, lemonades, applause and gratitude for many years to come, because thanks to his unrelenting effort Drupal is now finally consistent in this very security-sensitive area! 🤩🥳
- 🇬🇧United Kingdom alexpott 🇪🇺🌍
Left a comment on the MR. I think we need to always mark for reindex. The reindexing is not language specific but doing this is not going to recreate any row.
Test for this by expanding \Drupal\form_test\Form\FormTestDetailsForm to try with Claro in addition to the default test theme, and assert the expected class is present.
We are making changes in claro templates only.Do we need to do the same of default theme and then expand tests or shall we create a follow-up issue for default theme?
- 🇮🇳India sukr_s
changed to postsave and invoking
node_reindex_node_search($this->id());
only if the translation was not deleted. - @utkarsh_33 opened merge request.
- First commit to issue fork.
- 🇦🇺Australia kim.pepper 🏄♂️🇦🇺Sydney, Australia
📌 [PP-1] Refactor JSON-API file uploads to use FileUploadHandler Postponed Just landed so I think we are safe to mark this fixed now.
Thanks to everyone here for the years of work and support to get us to this point. Much appreciated. 🫶
No doubt there will be follow up issues to further refine and improve the code re-use here, but I think we are now in a much more secure situation that before.
- 🇬🇧United Kingdom alexpott 🇪🇺🌍
Committed and pushed 01da0fd920 to 11.x and 5923aeaed8 to 11.0.x and fa05fb7a93 to 10.4.x and 2d32ea4961 to 10.3.x. Thanks!
-
alexpott →
committed 01da0fd9 on 11.x
Issue #3371937 by yash.rode, Abhijith S, smustgrave, fgm: Theme...
-
alexpott →
committed 01da0fd9 on 11.x
-
alexpott →
committed 5923aeae on 11.0.x
Issue #3371937 by yash.rode, Abhijith S, smustgrave, fgm: Theme...
-
alexpott →
committed 5923aeae on 11.0.x
-
alexpott →
committed fa05fb7a on 10.4.x
Issue #3371937 by yash.rode, Abhijith S, smustgrave, fgm: Theme...
-
alexpott →
committed fa05fb7a on 10.4.x
-
alexpott →
committed 2d32ea49 on 10.3.x
Issue #3371937 by yash.rode, Abhijith S, smustgrave, fgm: Theme...
-
alexpott →
committed 2d32ea49 on 10.3.x
- 🇬🇧United Kingdom alexpott 🇪🇺🌍
Committed and pushed 655cb760fc to 11.x and 0bc782b1f8 to 11.0.x and 24b4a8ea8e to 10.4.x and 04fb2d1d1e to 10.3.x. Thanks!
diff --git a/core/modules/media/src/Plugin/Field/FieldFormatter/OEmbedFormatter.php b/core/modules/media/src/Plugin/Field/FieldFormatter/OEmbedFormatter.php index 132248d8fc..70047c8983 100644 --- a/core/modules/media/src/Plugin/Field/FieldFormatter/OEmbedFormatter.php +++ b/core/modules/media/src/Plugin/Field/FieldFormatter/OEmbedFormatter.php @@ -22,7 +22,6 @@ use Drupal\media\Plugin\media\Source\OEmbedInterface; use Symfony\Component\DependencyInjection\ContainerInterface; -// cspell:ignore allowtransparency /** * Plugin implementation of the 'oembed' formatter. *
Made the change above on commit because allowtransparency is no longer in the file.
-
alexpott →
committed 655cb760 on 11.x
Issue #3071446 by marcvangend, Kirst25, dcam, guptahemant, Lendude, nod_...
-
alexpott →
committed 655cb760 on 11.x
-
alexpott →
committed 0bc782b1 on 11.0.x
Issue #3071446 by marcvangend, Kirst25, dcam, guptahemant, Lendude, nod_...
-
alexpott →
committed 0bc782b1 on 11.0.x
-
alexpott →
committed 24b4a8ea on 10.4.x
Issue #3071446 by marcvangend, Kirst25, dcam, guptahemant, Lendude, nod_...
-
alexpott →
committed 24b4a8ea on 10.4.x
-
alexpott →
committed 04fb2d1d on 10.3.x
Issue #3071446 by marcvangend, Kirst25, dcam, guptahemant, Lendude, nod_...
-
alexpott →
committed 04fb2d1d on 10.3.x