mstrelan → made their first commit to this issue’s fork.
Re #43 we have this for node list now since ✨ Add 'View' to operations links where available Active
Rebased and fixed a conflict with 📌 Disable components by default sourced from block plugins provided by core in the Standard profile Active
mstrelan → made their first commit to this issue’s fork.
Thanks for the pointers. Removing the top and left styles resolves this.
The other difference I see when removing the picker style completely is that the browser allows the open select list to extend outside of the browser window if there is space available on the screen. This feels much nicer. I'd be in favour of removing it entirely, but could settle on just removing the negative margins.
Oh I found it.
In \Drupal\KernelTests\KernelTestBase::tearDown
// Delete test site directory.
$callback = function (string $path) {
@chmod($path, 0700);
};
This is passed to \Drupal\Core\File\FileSystem::deleteRecursive and it's applied to every file. Perhaps we could just fix that to now chmod symlinks? I'm curious why we need to chmod a file that is going to be deleted anyway, if we have permission to set the permissions then surely we can just delete it?
If I'm following this right, the code in question is this:
foreach (scandir("core/modules/user/tests/modules/user_hooks_test") as $item) {
$target = "$this->siteDirectory/$item";
if (!file_exists($target)) {
symlink(realpath("core/modules/user/tests/modules/user_hooks_test/$item"), $target);
}
}
For a start, symlink() expects $target as the first parameter, but I think we're just using the wrong name for the variable. The target is where the symlink points to, which is the file in core/modules. The link we're creating is written to $this->siteDirectory which should be in sites/simpletest, which should be gitignored.
If I'm mistaken, can you point to where the test is writing to a file that is checked in to the repository?
Tested by making the same change to core/phpcs.xml.dist and running composer phpcs. Compared the list of reported files with the MR and confirm they match up. PHPCS job is passing now so confirming that is resolved. Spot checked the changes for any discrepancies but all looks ok, including odd looking test cases that have strings beginning with white space characters. Also grepped in core/tests/Drupal/Tests/Component for "phpcs" and "@codingStandardsIgnore" to see if there were any ignores to remove but there were not.
Rebased and resolved conflict due to 🐛 Why does DbImportCommandTest install modules? Active
Rebased and resolved conflicts due to 📌 Fix LongLineDeclaration in Functional tests Active
Added the CR. Note it only affects classes that extend a base class using the trait and override this method.
Have opened an MR for 11.2.x, although not sure if should backport this, since we didn't do the same for #3547124: Remove system module from kernel tests that don't need it → , which is the source of the conflict.
I put up an alternative more advanced solution using @phpstan-type and @phpstan-import-type in MR !13352. Not sure if core is ready for this though so feel free to run with the original if that's desired.
Added some comments, I think at least we need to update the return description. Maybe add @see \Drupal\user\PermissionHandlerInterface::getPermissions explaining that's what a permission looks like.
OK, I was trusting the MR overview page saying it merges cleanly. I tested manually and can confirm it applies with patch -p1 but not with git apply.
$ git fetch && git reset --hard origin/11.x
HEAD is now at 5cbac86546d
✨
Add 'View' to operations links where available
Active
feat: Add 'View' to operations links where available
$ curl -s https://git.drupalcode.org/project/drupal/-/merge_requests/13290.diff | git apply
error: patch failed: core/modules/system/tests/src/Kernel/Scripts/DbImportCommandTest.php:16
error: core/modules/system/tests/src/Kernel/Scripts/DbImportCommandTest.php: patch does not apply
$ curl -s https://git.drupalcode.org/project/drupal/-/merge_requests/13290.diff | patch -p1
patching file core/modules/system/tests/src/Kernel/Scripts/DbImportCommandTest.php
Hunk #1 succeeded at 17 with fuzz 1 (offset 1 line).
Have rebased and force pushed now.
I think there is a bit of boilerplate we can remove, have marked up in the MR. Also wondering about a base class, see comment in the MR. These are not blockers though so I'd leave the status as NR, except I think the $session->wait() needs attention, so setting to NW.
mstrelan → changed the visibility of the branch 3524379-history to hidden.
Rebased and replicated changes from CommentLinkBuilderTest to the new HistoryCommentLinkBuilderTest
Currently it passes with 384 MB so the current requirement is somewhere between 256 and 384. If we set it to -1 and the process starts requiring more than 512 MB (or possibly much more) then that's a good indicator we might have a problem. Maybe phpcs started scanning node_modules or vendor or something like that.
Rebased for the changes in 📌 Deprecate comment libraries and move to history module Active but this is now postponed on 📌 Deprecate route comment.new_comments_node_links Active
Clean rebase, not sure why the bot complained. Restoring RTBC as I didn't do anything else.
$ git fetch origin
$ git rebase origin/11.x
Successfully rebased and updated refs/heads/3542528-deprecate-route-comment.newcommentsnodelinks.
Added my thoughts to the MR. Setting NW because I think the copied tests should be trimmed down in this issue.
#13 - please see the parent issue / meta, the approach can be discussed there. TL;DR a new text_with_summary module is created for the text_with_summary field type, the module is then moved to contrib and deprecated in core. Then we remove it from core in the next major. This allows sites that rely on this field type to keep using it if they want to.
Some of the tests were random fails, I fixed a couple more, and there are some remaining.
This is pretty straightforward, have put up an MR that fixes it.
I can reproduce this on 11.x with the example module in #5. Updating steps to reproduce.
I tested 3535024-fix-with-window and it seems to be working well. The issue I had was with a component that has a date field and a multi-value tags field (with #3546869). Using the "add another item" or "remove" button on the tags would occasionally result in Error 500: DateTime::createFromFormat(): Argument #2 ($datetime) must be of type string, array given. I haven't been able to reproduce the error with MR !122 applied.
This is not so much about restricting at the moment, but about providing a default component in a slot. For example, adding a Slide Deck component would automatically add one (or more) Slide components in the slides slot. Assuming the Slide has its own default values for its fields, we don't need to define those in the Slide Deck.
This needs a rebase, but also I'm not sure we should include the most recent commits. There's a reason I left them out originally, in fact I commented about them in #12 so this is rather counter-productive.
Needs work for #5, feel free to work on that.
Re: tests-only, the way it works is to do get a diff of any tests, apply it to 11.x / HEAD and re-run the tests. In this case the enum we've introduced doesn't exist in HEAD so of course it fails. As @smustgrave said, it's usually for bugs, which usually don't need new classes.
mstrelan → changed the visibility of the branch 3513326-add-return-type to hidden.
This looks right to me, we might want test coverage though.
Because this is a change to configuration, it will need an update path so existing sites will get the changed configuration. That update path will also need to be tested.
Not sure we normally update existing sites for things like this, we didn't in
📌
Use easily recognizable date format
RTBC
. We possibly could here since it's a bug, but I think we should only do it if the format has not already been customised on the site. So we'd have to load the format and only update it if it matches Y-\WW.
I'm fairly certain this issue will need a change record, but I could be wrong.
I'm not sure it's necessary here if we're treating this as a bug fix, especially given the rarity of it.
Finally, the issue summary needs to be updated with all of the headings from the issue summary template. You may omit text under headings that are irrelevant to the issue or enter "N/A." It also needs to have thorough steps to reproduce the issue starting from a vanilla install of Drupal. Doing this ensures that someone who is reviewing the issue later on can check it quickly without needing to do any guesswork.
Agree it could use an update, but I don't think we need manual steps to reproduce if it's easily reproducible in the failing test like we have here.
FWIW I ran the script against all core modules and have the count of tests that could be updated below. If we want to remove all of these I'm happy to open up issues for these, either all at once or the bigger ones first. But waiting to see if anyone thinks it's worthwhile.
ban: 1
basic_auth: 1
block: 7
block_content: 3
comment: 9
config: 4
config_translation: 5
contact: 3
content_translation: 16
datetime: 4
datetime_range: 2
dblog: 3
editor: 1
field: 40
field_ui: 3
file: 21
filter: 21
history: 2
image: 3
jsonapi: 7
language: 4
layout_builder: 1
layout_discovery: 4
link: 3
locale: 14
media: 1
media_library: 1
menu_link_content: 3
menu_ui: 9
migrate: 5
migrate_drupal: 3
node: 14
options: 4
path: 2
path_alias: 4
rest: 1
search: 3
serialization: 2
shortcut: 1
syslog: 1
taxonomy: 7
telephone: 1
text: 21
update: 1
user: 45
views: 19
workspaces: 1
I must have missed it in debugging, but initial preprocess is right there for us to inspect. Pushed a fix for this.
The test fails because its expecting template_preprocess_layout to exist in $theme_definitions['test_layout_theme']['preprocess functions']. This doesn't exist because we use initial preprocess now instead. There is a snippet in \Drupal\Core\Theme\Registry::processExtension that is responsible here:
// Add template_preprocess_HOOK function, if no initial preprocess
// callback is defined.
if (empty($info['initial preprocess']) && function_exists('template_preprocess_' . $hook)) {
// @todo trigger deprecation in https://www.drupal.org/project/drupal/issues/3513595.
$info['preprocess functions'][] = 'template_preprocess_' . $hook;
}
I suspect we don't need testThemeProvidedLayout anymore, or we need something else to verify initial preprocess is set or invoked, specifically for layouts in themes.
Sure, I opened 📌 Remove user module from kernel tests that don't need it Needs review but have some concerns that I've raised on the MR.
A lot of these are just duplicating what's in the base class, so removing it is not actually removing it. But there are some where it appears the user module should be required, but isn't, and some where there's no need for user module at all. I've added some comments in the MR, only checked the first 10 or so files.
Re #5 does that matter? As per #6 these tests don't need system module installed, or in some cases it's already installed in a parent class. IMHO tests should seek to do the minimum amount of setup as possible to make the test pass.
Converted to meta and opened #3547349: Move ANONYMOUS_ constants in CommentInterface to new AnonymousContact enum → to start
@ericimagexmedia let's limit the scope of this issue to phpdoc only. Adding actual native return types can have backwards compatibility considerations.
Updated to kernel tests only. Functional tests get system module automatically, but some tests should explicitly require it.
I don't really understand the Remaining Tasks. Surely there are some tests that do need the system module? I think we could potentially have a follow up to find tests that have system plus some other modules and see which ones can have system removed.
Updated one function doc to better explain the structure.
Updated the proposed resolution to delete "Translate the fallback category (module name) for other blocks". Updated the MR as per #58.
Nevermind, this is a duplicate of 📌 Update static prop source method names to reflect their stability Active
From my perspective, history module is going to contrib where the maintainer can do whatever they want. I'd rather do the minimal amount to deprecate it. The suggested changes would make the tests make more sense on their own, but I didn't want to deviate too much from the original tests to keep the scope down.
FWIW phpstan level 3 would detect this. We could potentially expand this issue to fix all instances of functions that return false when something else is expected?
There are 27 instances:
$ ./vendor/bin/phpstan analyse -c core/phpstan.neon.dist --level=3 --error-format=raw | grep "returns false" | sed "s|$PWD|.|g"
10764/10764 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%
./core/includes/install.core.inc:1476:Function install_check_localization_server() should return string but returns false.
./core/lib/Drupal/Core/Asset/AssetDumper.php:57:Method Drupal\Core\Asset\AssetDumper::dumpToUri() should return string but returns false.
./core/lib/Drupal/Core/Asset/AssetDumper.php:61:Method Drupal\Core\Asset\AssetDumper::dumpToUri() should return string but returns false.
./core/lib/Drupal/Core/Asset/AssetDumper.php:73:Method Drupal\Core\Asset\AssetDumper::dumpToUri() should return string but returns false.
./core/lib/Drupal/Core/Asset/AssetDumper.php:77:Method Drupal\Core\Asset\AssetDumper::dumpToUri() should return string but returns false.
./core/lib/Drupal/Core/Batch/BatchStorage.php:62:Method Drupal\Core\Batch\BatchStorage::load() should return array but returns false.
./core/lib/Drupal/Core/Config/FileStorage.php:203:Method Drupal\Core\Config\FileStorage::decode() should return array but returns false.
./core/lib/Drupal/Core/ImageToolkit/ImageToolkitManager.php:84:Method Drupal\Core\ImageToolkit\ImageToolkitManager::getDefaultToolkit() should return Drupal\Core\ImageToolkit\ImageToolkitInterface but returns false.
./core/lib/Drupal/Core/Password/PhpPassword.php:34:Method Drupal\Core\Password\PhpPassword::hash() should return string but returns false.
./core/lib/Drupal/Core/Password/PhpassHashedPasswordBase.php:130:Method Drupal\Core\Password\PhpassHashedPasswordBase::crypt() should return string but returns false.
./core/lib/Drupal/Core/Password/PhpassHashedPasswordBase.php:137:Method Drupal\Core\Password\PhpassHashedPasswordBase::crypt() should return string but returns false.
./core/lib/Drupal/Core/Password/PhpassHashedPasswordBase.php:144:Method Drupal\Core\Password\PhpassHashedPasswordBase::crypt() should return string but returns false.
./core/lib/Drupal/Core/Password/PhpassHashedPasswordBase.php:149:Method Drupal\Core\Password\PhpassHashedPasswordBase::crypt() should return string but returns false.
./core/lib/Drupal/Core/Theme/ThemeManager.php:185:Method Drupal\Core\Theme\ThemeManager::render() should return Drupal\Component\Render\MarkupInterface|string but returns false.
./core/lib/Drupal/Core/Updater/Updater.php:136:Method Drupal\Core\Updater\Updater::findInfoFile() should return string but returns false.
./core/modules/content_translation/src/ContentTranslationHandler.php:275:Method Drupal\content_translation\ContentTranslationHandler::getSourceLangcode() should return string but returns false.
./core/modules/file/tests/file_test/src/StreamWrapper/DummyExternalReadOnlyWrapper.php:50:Method Drupal\file_test\StreamWrapper\DummyExternalReadOnlyWrapper::realpath() should return string but returns false.
./core/modules/file/tests/file_test/src/StreamWrapper/DummyExternalReadOnlyWrapper.php:57:Method Drupal\file_test\StreamWrapper\DummyExternalReadOnlyWrapper::dirname() should return string but returns false.
./core/modules/file/tests/file_test/src/StreamWrapper/DummyExternalReadOnlyWrapper.php:141:Method Drupal\file_test\StreamWrapper\DummyExternalReadOnlyWrapper::stream_tell() should return int but returns false.
./core/modules/file/tests/file_test/src/StreamWrapper/DummyRemoteStreamWrapper.php:33:Method Drupal\file_test\StreamWrapper\DummyRemoteStreamWrapper::realpath() should return string but returns false.
./core/modules/locale/locale.translation.inc:187:Function locale_translation_source_check_file() should return object but returns false.
./core/modules/migrate/src/Annotation/MigrateSource.php:75:Method Drupal\migrate\Annotation\MigrateSource::getProvider() should return string but returns false.
./core/modules/views/src/Plugin/views/query/Sql.php:480:Method Drupal\views\Plugin\views\query\Sql::addTable() should return string but returns false.
./core/modules/views/src/Plugin/views/query/Sql.php:528:Method Drupal\views\Plugin\views\query\Sql::queueTable() should return string but returns false.
./core/modules/views/src/Plugin/views/query/Sql.php:563:Method Drupal\views\Plugin\views\query\Sql::queueTable() should return string but returns false.
./core/modules/views/src/Plugin/views/query/Sql.php:642:Method Drupal\views\Plugin\views\query\Sql::ensureTable() should return string|null but returns false.
./core/modules/views/src/ViewExecutable.php:1717:Method Drupal\views\ViewExecutable::preview() should return array|null but returns false.
Actually we don't need to postpone this, it can happen first, or at any time.
If we're going with this approach of installing the modules in the base class then we should remove the $modules property from the child classes.
I think we need @catch to weigh in here. I think it might be better to focus on the work in 📌 Remove calls to system_page_attachments() Postponed and see if there's anything left to do here.
@smustgrave I don't think so, I followed the Asset Libraries section of How to deprecate → .
Not sure about #10. I took the test from the patch in #4 and applied the fix from 🐛 Denormalization of empty string date throws UnexpectedValueException Needs work but the test still failed. That's because TimestampNormalizer::denormalize still tries to call ::getTimestamp on null.
Given there have been no responses since this was triaged 3 years ago, and there has been no other activity in 11 years, I think we can close this.
Resolved merge conflict due to annotations to attributes conversion in tests. Updated the new test class to also use attributes.
I'm not sure about #6, because I think a route alias should have the same path, or at least it does for router_test.deprecated.
Also not sure it works if history module is not installed and the route doesn't exist, although the controller throws a 404 in that case anyway.
I did notice that the deprecation format was incorrect though, so I've fixed that up.
The issue title "Ajax enabled Webform ignores client side form validation" can be reproduced following #18. I think you're saying in #19 that's not possible, so I guess this should be wont fix / works as designed?
Can confirm my concerns in #7 have been addressed. Have read through the MR and confirmed a one-to-one mapping of annotation to attributes here.
Thanks for that. I looked through all the classes calling FileSystem::basename to ensure there were no leftover filesystem properties that were no longer in use. Can confirm there are none remaining.
If the php language level in phpstorm is 8.2 or above (when readonly classes were introduced) and all properties on the class are marked readonly, then it will suggest that the class can be readonly.
Yeah I've considered the same. If the class is readonly then all properties are readonly, including those of child classes. I believe that would be fine, but given there is pushback for having final classes then I guess we shouldn't restrict what they can do either?
Looked at the first two files, aren't we losing some annotations that haven't been converted yet here?