Account created on 12 June 2008, almost 16 years ago
#

Merge Requests

More

Recent comments

πŸ‡¦πŸ‡ΊAustralia mstrelan

FWIW there is a rector rule AddOverrideAttributeToOverriddenMethodsRector for adding the attribute. It won't remove the @inheritdoc annotation though.

πŸ‡¦πŸ‡ΊAustralia mstrelan

Perhaps as a starting point we could just add the frankenphp static binary to an existing image (like drupalci/php-8.2-cli) and work to build our own image over time.

We also need to configure a valid SSL certificate or disable HTTPS by setting the env var SERVER_NAME=:80.

πŸ‡¦πŸ‡ΊAustralia mstrelan

way less failures than i feared. I have a problem installing a site too so that makes sense the tests fail, thanks for testing that!

I hadn't run FunctionalJavascript tests yet but now that I've started to run some they seem to mostly be passing, only failing on similar issues to the Functional tests.

Started having a look at the first of those fails mentioned in #29. It's expecting a 422 but getting a 403. The interesting thing is that if I repeat the test 10 times it passes 5 five times. So it's definitely capable of sending a 422, but for some reason it's not consistent.

$ phpunit -c core/phpunit.xml.dist core/modules/ckeditor5/tests/src/Functional/ImageUploadTest.php  --filter=testUploadFileExtension --repeat=10
PHPUnit 9.6.19 by Sebastian Bergmann and contributors.

Testing 
F.FF...F.F

It's possible that some of random files created by $this->getTestFiles are causing issues and others aren't. Would need to investigate a little further and try to remove the randomness.

I'm nowhere regarding the gitlab ci stuff in #3438767: Support FrankenPHP as a webserver I don't really have time to spend on it and I'm not even sure where to start so any help would be welcome :)

I've also been thinking about gitlab ci but need some more time. Perhaps as a starting point we could just add the frankenphp static binary to an existing image (like drupalci/php-8.2-cli) and work to build our own image over time

πŸ‡¦πŸ‡ΊAustralia mstrelan

I've been running core's functional tests with FrankenPHP and this Caddyfile on my localhost and getting pretty good results. At first there were many failing tests due to the self signed certificate, but disabling HTTPS resolved those. Currently I'm down to the below list of fails:

Unexpected status code

  • \Drupal\Tests\ckeditor5\Functional\ImageUploadTest::testUploadFileExtension - getting 403 instead of 422
  • \Drupal\Tests\ckeditor5\Functional\ImageUploadAccessTest::testCkeditor5ImageUploadRoute - getting 403 instead of 201
  • \Drupal\Tests\ckeditor5\Functional\ImageUploadAccessTest - getting 403 instead of 422
  • \Drupal\Tests\file\Functional\DownloadTest::testFileCreateUrl - getting 404 instead of 200
  • \Drupal\Tests\rest\Functional\Views\StyleSerializerTest::testResponseFormatConfiguration - getting 200 instead of 406
  • \Drupal\Tests\system\Functional\CsrfRequestHeaderTest::testRouteAccess - getting 200 instead of 403
  • \Drupal\Tests\system\Functional\System\HtaccessTest::testFileAccess - getting 200 instead of 403

Unexpected http headers

  • \Drupal\FunctionalTests\WebAssertTest::testResponseHeaderExists - Failed asserting that the response has a 'Null-Header' header.
  • \Drupal\FunctionalTests\WebAssertTest::testResponseHeaderDoesNotExist - AssertionFailedError not thrown
  • \Drupal\Tests\jsonapi\Functional\MessageTest::testPostIndividual - unexpected allow header value
  • \Drupal\Tests\system\Functional\Routing\RouterTest::testFinishResponseSubscriber - no vary header
  • \Drupal\Tests\system\Functional\System\HtaccessTest::testSvgzContentEncoding - missing gzip for x-encoded-content-encoding

Unit test fails

  • \Drupal\Tests\Scripts\TestSiteApplicationTest::testInstallScript - BadRequestHttpException
  • \Drupal\Tests\Scripts\TestSiteApplicationTest::testInstallInDifferentLanguage - non-zero exit code

Other issues

  • \Drupal\Tests\mysql\Functional\Mysql8RequirePrimaryKeyUpdateTest::testDatabaseLoaded - missing db privileges
  • \Drupal\Tests\update\Functional\FileTransferAuthorizeFormTest::testViaAuthorize - page missing "Files were added successfully" message
  • \Drupal\Tests\update\Functional\UpdateUploadTest::testUploadModule - Current page is "/core/authorize.php/core/authorize.php", but "/core/authorize.php" expected.
πŸ‡¦πŸ‡ΊAustralia mstrelan

Thanks for creating the CRs. This looks good. Baseline may need to be regenerated when committing.

πŸ‡¦πŸ‡ΊAustralia mstrelan

We probably need a change record :D

πŸ‡¦πŸ‡ΊAustralia mstrelan

Does this still need framework manager review? Otherwise RTBC from me.

πŸ‡¦πŸ‡ΊAustralia mstrelan

Added tags for change record, deprecation tests

πŸ‡¦πŸ‡ΊAustralia mstrelan

This might not be possible due to the provider and class properties on AttributeBase. Leaving open in case anyone else has bright ideas.

πŸ‡¦πŸ‡ΊAustralia mstrelan

Let's just do the following patterns:

<include-pattern>*/tests/modules/*</include-pattern>
<include-pattern>*/tests/*_test/*</include-pattern>

Anything outside of this can be caught later when we limit to */tests/*.

πŸ‡¦πŸ‡ΊAustralia mstrelan

πŸ“Œ Standardize location of test modules Needs review is likely a won't fix, so we can continue with just the modules that match the patterns in the issue summary.

πŸ‡¦πŸ‡ΊAustralia mstrelan

Sounds fine to me, I think we can target */tests/modules/* and */tests/*_test/* and catch the outliers later.

πŸ‡¦πŸ‡ΊAustralia mstrelan

Opened πŸ“Œ Standardize location of test modules Needs review to address #20.

πŸ‡¦πŸ‡ΊAustralia mstrelan

Current status (assuming Kernel tests gets in soon):

<include-pattern>*/tests/*</include-pattern>
<exclude-pattern>*/tests/modules/*</exclude-pattern>
<exclude-pattern>*/tests/themes/*</exclude-pattern>
<exclude-pattern>*/tests/*_test/*</exclude-pattern>
<exclude-pattern>*/tests/*fixture*/*</exclude-pattern>
  • πŸ“Œ Add declare(strict_types=1) to all test modules Postponed should take care of the first two exclude patterns.
  • We need another issue for test themes, I think they are all in core/modules/system/tests/themes/
  • We need another issue for fixtures, although maybe we should continue to ignore them

That leaves the following that need to be categorised:

core/modules/comment/src/Tests/CommentTestTrait.php
core/modules/contact/tests/drupal-7.contact.database.php
core/modules/filter/tests/filter_test_plugin/src/Plugin/Filter/FilterSparkles.php
core/modules/filter/tests/filter_test_plugin/src/Plugin/Filter/FilterTestStatic.php
core/modules/menu_link_content/tests/menu_link_content_dynamic_route/src/Routes.php
core/modules/migrate_drupal/src/Tests/StubTestTrait.php
core/modules/node/tests/node_access_test_auto_bubbling/src/Controller/NodeAccessTestAutoBubblingController.php
core/modules/system/tests/http.php
core/modules/system/tests/https.php
core/modules/views/src/Tests/AssertViewsCacheTagsTrait.php
core/modules/views/src/Tests/TestHelperPlugin.php
core/modules/views/src/Tests/ViewResultAssertionTrait.php
core/modules/views/src/Tests/ViewTestData.php
core/tests/bootstrap.php
  • Possibly the Traits and Views classes should move to the tests dir in each module, unless there is some reason they need to live in src/Tests instead.
  • We should make sure to capture filter_test_plugin, menu_link_content_dynamic_route and node_access_test_auto_bubbling in πŸ“Œ Add declare(strict_types=1) to all test modules Postponed , or move them to a test/modules directory first
  • Not too sure about the remaining non-class php files, I guess we should be ok to include them
πŸ‡¦πŸ‡ΊAustralia mstrelan

This is great, thanks!

πŸ‡¦πŸ‡ΊAustralia mstrelan

Since this is the last of the test types to have strict types enabled we can loosen the phpcs pattern.

Before:

<include-pattern>*/tests/src/Functional/*</include-pattern>
<include-pattern>*/tests/src/FunctionalJavascript/*</include-pattern>
<include-pattern>*/tests/src/Kernel/*</include-pattern>
<include-pattern>*/tests/src/Traits/*</include-pattern>
<include-pattern>*/tests/src/Unit/*</include-pattern>

After:

<include-pattern>*/tests/src/*</include-pattern>

Note that since πŸ“Œ Fix strict type errors in AssertContentTrait Needs work is still outstanding we need to exclude that one file. I think it's better to get this in sooner while this is green rather than waiting to resolve that one remaining issue. Adding the following:

<!-- @todo remove this in https://www.drupal.org/project/drupal/issues/3402707 -->
<exclude-pattern>./tests/Drupal/KernelTests/AssertContentTrait.php</exclude-pattern>
πŸ‡¦πŸ‡ΊAustralia mstrelan

There's an open thread on the MR that I don't know the answer to. Also the build is failing so I obviously didn't follow the correct procedure to update composer.json.

πŸ‡¦πŸ‡ΊAustralia mstrelan

This is unblocked now. The MR at https://git.drupalcode.org/project/drupal/-/merge_requests/6826 demonstrates that adding these fixes and declaring strict types everywhere results in a green pipeline. So if we get this in we can update πŸ“Œ [PP-2] Add declare(strict_types=1) to all Kernel tests Postponed and get that in also.

πŸ‡¦πŸ‡ΊAustralia mstrelan

I guess if we add this in 10.2.x it will just mean folks without zlib will get stuck on 10.2.5 which will also be broken for them. So possibly we need a runtime check in 10.2.6 (or later) to handle it gracefully.

πŸ‡¦πŸ‡ΊAustralia mstrelan

Yeah you need to install it. The patch will just prevent people upgrading of they don't have it installed

πŸ‡¦πŸ‡ΊAustralia mstrelan

Actually this is a bug, because we don't explicitly declare a dependency on ext-zlib, but installing the extension should resolve this for you.

πŸ‡¦πŸ‡ΊAustralia mstrelan

Seems your php version does not have the zlib extension installed.

πŸ‡¦πŸ‡ΊAustralia mstrelan

mstrelan Thank you for contributing both MRs with test coverage. One side note: I appreciate that you are using KernelTest, which has not been used much in the webform module's test coverage.

No problem, I think in hindsight there are some existing functional tests that could be extended but I have backported this from the client project I'm working on and it's primarily to demonstrate what the patch does, assuming if it eventually makes it to webform it would go through some refactoring.

It is challenging that a "multipart/form-data" post could/should have two different structures for array values. We are also not addressing how files would be included in a "multipart/form-data" post.

I'm wondering if one way is more correct than the other but really only have the one use case to go by. I think if there was a general consensus on one or the other then we could proceed, but if not then it's probably not worthwhile. I have completely overlooked files, which I think was the original intent of this issue, sorry for that.

Creating a new handler, which could extend the RemotePost handler, in a dedicated contributed module could be more straightforward to address the "multipart/form-data" post and the Freshdesk.com customer support software.

Exactly, and this is what we have ended up doing as we have other massaging to do in the handler as well. But the biggest challenge with this is that the remotePost method is 80 lines long, so without patching webform we have to duplicate the whole function. It would be easier to extend if this was broken up in to smaller chunks. Having said that there is probably a fair bit that is not applicable for our custom handler and could be removed, such as how to handle the GET method, since we'll only use POST.

πŸ‡¦πŸ‡ΊAustralia mstrelan

Added two MRs and hid the patch. MR !449 is in the original format I attempted as per previous comments and MR !450 is in the format that actually worked for Consultation Manager. Test coverage demonstrates the converted formats.

πŸ‡¦πŸ‡ΊAustralia mstrelan

It doesn't appear that there is a clean way to hook in to drush generators. You can see the generator was added in Add bundle classes generator #4903, we'd probably need override the service class, or possibly decorate it with a symfony service decorator. I don't really want to maintain the logic for that and twig files for generation. Ultimately if ✨ Define bundle classes via attributes Needs work made it in to core then we could shift this responsibility to Drush, therefore this is probably "won't fix" but will leave it open for discussion for now.

πŸ‡¦πŸ‡ΊAustralia mstrelan

Thank you @quietone, have looked over the last 3 commits and am happy with these changes.

πŸ‡¦πŸ‡ΊAustralia mstrelan

I suspect "Optimize imports" depends on other configuration such as inspections and code style. For example there is an option to sort use statements alphabetically or by length (who would use that?). Possibly it even looks at phpcs configuration.

πŸ‡¦πŸ‡ΊAustralia mstrelan

I agree with #2 and came here to echo the sentiments about this bring a constructor call. I think we should enforce named parameters and multi line parameters wherever we have more than just an id. Having this fixable via phpcbf would make the rector rule to convert from annotations a lot more useful.

πŸ‡¦πŸ‡ΊAustralia mstrelan

Thanks for this @RichardGaunt

Did you mean to open a merge request for the issue fork or is this still in progress? I think we could add the .gitlab-ci.yml from the template as well while we're at it.

πŸ‡¦πŸ‡ΊAustralia mstrelan

Not sure what the expected format is but for Consultation Manager we needed multi value fields (like checkboxes) to have the same form key without [] rather than an array of values.

πŸ‡¦πŸ‡ΊAustralia mstrelan

I'm not overly fussed, but we now have code that looks like this:

// Set the machine name to the transliterated value.
if (machine !== '') {
  // ...
  data.$suffix[0].style.display = 'block';
} else {
  data.$suffix.hide();
  // ...
}

Where the truthy part of the if condition sets the style directly but the falsey part uses jquery hide.

Besides, I haven't looked thoroughly, but how do we know 'block' is the right value, maybe it should be 'inline-block' or 'inline'? IIRC jquery takes care of that.

πŸ‡¦πŸ‡ΊAustralia mstrelan

Does this need a dependency evaluation also?

πŸ‡¦πŸ‡ΊAustralia mstrelan

I don't want to change the scope, will almost instantly open a follow up if this goes in, but level 3 gives us:

return types, types assigned to properties

On all new code, which most code reviews are picking up anyway, but would be much nicer if it was for free.

πŸ‡¦πŸ‡ΊAustralia mstrelan

Level 2 gives you:

unknown methods checked on all expressions (not just $this), validating PHPDocs

See https://phpstan.org/user-guide/rule-levels

Or more specifically see https://github.com/phpstan/phpstan-src/blob/1.11.x/conf/config.level2.neon

πŸ‡¦πŸ‡ΊAustralia mstrelan

I think we need to deprecate the consts and keep them around. Also seems we are likely to move to UpperCamelCase for enums as per #3339746: Decide on a coding style for PHP Enumerations β†’ .

πŸ‡¦πŸ‡ΊAustralia mstrelan

Just pointing out that phpstan-prophecy 1.0.1 has been released and the MR has been updated to that version, so the concerns in #8 about the dev release are no longer applicable. Let's get this in.

πŸ‡¦πŸ‡ΊAustralia mstrelan

This issue feels it should have a meta for discussing the merits of frankenphp and another issue for gitlab ci, so we can focus this issue on the caddyfile.

πŸ‡¦πŸ‡ΊAustralia mstrelan

I guess that's supposed to be sarcasm. Seems that we need to take 309 and add the test from 305 and review against that. A merge request would probably be helpful. The hook_requirements in 296 hasn't been addressed either I don't think.

πŸ‡¦πŸ‡ΊAustralia mstrelan

I think part of dropping IIS and Windows support is that we can't test on it. Is it possible, or is there an existing issue, where we can run caddy in gitlab ci?

πŸ‡¦πŸ‡ΊAustralia mstrelan

I did an IRL test with redirect.module looking at \Drupal\Core\Plugin\ContainerFactoryPluginInterface::create which we added the string type declaration to the $plugin_id param. The implementation \Drupal\redirect\Plugin\Action\DeleteRedirect::create does not have param types but I was able to delete a redirect with bulk actions. I then added static return type to the function on the interface and got a fatal error. Removed the return type and added it to the concrete class and it worked fine.

I also had a play with expanding this to included non-scalar types, union types and nullable types but ran in to a number of issues. For example arrays would always be represented with generics syntax. Objects would not add namespaces correctly so I skipped them. Union types had the same issues, although I was somewhat able to get them working for scalar types. Finally nullable types worked ok except they were represented as a union type instead of the usual nullable syntax we use.

Would it be helpful to break this down into bite-sized chunks?

Happy to split it in to something that's logical if we think that's going to be useful, although @catch mentioned in #3050720-47 that one big issue might be ok. So if there are any suggestion for how this should be split let me know and I'll see what I can do.

πŸ‡¦πŸ‡ΊAustralia mstrelan

Great stuff! I think we need to remove the rector bits from the MR before it's ready to commit, but they are worth keeping around until then.

πŸ‡¦πŸ‡ΊAustralia mstrelan

Not too many failures, mostly due to mocking things. Probably a lot of these params need to be nullable.

πŸ‡¦πŸ‡ΊAustralia mstrelan

mstrelan β†’ changed the visibility of the branch 3436327-string-type-rector to hidden.

πŸ‡¦πŸ‡ΊAustralia mstrelan

Added another MR that applies only to interfaces and converts all scalar param types based on their phpdoc. Rescoping issue.

πŸ‡¦πŸ‡ΊAustralia mstrelan

If I'm understanding the comment properly, it's safe with regards to sub classing, but any calling code could throw an exception, right?

I think you're right, so in that case we'd want to limit this to interfaces only.

πŸ‡¦πŸ‡ΊAustralia mstrelan

I did a simple test modifying the existing \Rector\Php80\Rector\FunctionLike\MixedTypeRector and replacing checks for mixed and MixedType with string and StringType. This was successfully able to detect untyped params that had string types in their docs and add the string type declaration. We could certain rinse and repeat for primitive types like int and bool, and I'm sure with a bit more time we could make this generic across all types.

πŸ‡¦πŸ‡ΊAustralia mstrelan

If we can script things so that we can apply type hints based on the phpdoc, then one big issue sounds great.

FWIW rector has a set of existing rules for adding type declarations, for example AddParamTypeFromPropertyTypeRector for getters and setters. Unfortunately there doesn't seem to be a rule for AddParamTypeFromPhpDoc or similar, but there are a bunch of helpers in there for inspecting php docs. I think if we were to script this we should start by contributing the rule to rector.

πŸ‡¦πŸ‡ΊAustralia mstrelan

Confused about what exactly was the problem. #2827055-141: Add option to show only start or end date in the DateTime Range custom formatter β†’ points to https://stitcher.io/blog/new-in-php-82#fetch-properties-of-enums-in-cons... but that is about fetching enum properties in const expressions, which I couldn't find any instances of in the revert commit.

Regardless, I think this issue needs more information about what we're trying to do here. Something like "Convert DateTimeRangeConstants interface to DateTimeRangeDisplayOptions enum".

Guess it's also postponed until ✨ Add option to show only start or end date in the DateTime Range custom formatter Needs review is back in.

πŸ‡¦πŸ‡ΊAustralia mstrelan

I generated a baseline for each level from 1-6 so see where is the biggest jump.

level 1: 630 errors
level 2: 8415 errors
level 3: 10657 errors
level 4: 14012 errors
level 5: 16015 errors
level 6: 64641 errors

I think if we're happy with a baseline of ~8000 errors at level then we should aim a little higher, perhaps level 3, or, if there is appetite, then 4 or 5.

πŸ‡¦πŸ‡ΊAustralia mstrelan

This is what I had in mind, happy to be told it's not worth it, just thought it was worth considering.

https://git.drupalcode.org/project/drupal/-/merge_requests/7178

πŸ‡¦πŸ‡ΊAustralia mstrelan

Unfortunately that's not enough, we'd need a backwards compatible call in PasswordConstraintPluginManager::construct to parent::construct. Currently on 10.1 we get something like this when enabling the module:

TypeError: Drupal\Core\Plugin\DefaultPluginManager::__construct(): Argument #6 ($additional_annotation_namespaces) must be of type array, string given, called in /data/app/modules/contrib/password_policy/src/PasswordConstraintPluginManager.php on line 27 in Drupal\Core\Plugin\DefaultPluginManager->__construct() (line 130 of /data/app/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php).
πŸ‡¦πŸ‡ΊAustralia mstrelan

Wonder if we'd be better off making RfcLogLevel in to an enum and move the mapping to a function there. Setting NR for the idea, please set to NW if you agree or back to RTBC if not.

πŸ‡¦πŸ‡ΊAustralia mstrelan

FWIW this would need to be in a branch that requires Drupal 10.2 or higher.

πŸ‡¦πŸ‡ΊAustralia mstrelan

I think we missed a few deprecations because I was searching for "removed from drupal:11" but there are a number of instances of "removed in drupal:11", "required in drupal:11" and "will cause an error in drupal:11". The reason I didn't search for @deprecated is because it also matched drupal:12.

Adding the following to the issue summary:

  • UserRequestSubscriber::__construct
  • UserController::__construct
  • User::hasPermission
  • PermissionHandler::__construct
  • UserLoginBlock::__construct
πŸ‡¦πŸ‡ΊAustralia mstrelan

This looks good to me. We've removed all the deprecated functions in node.module and the deprecated constructor param in NodeRevisionDeleteForm, and the tests are all passing. Arguably this could be combined with πŸ“Œ Remove deprecated actions from node module Active but not sure it's worth the effort now that it's already split.

πŸ‡¦πŸ‡ΊAustralia mstrelan

Aiming to work on this tomorrow at Drupal South Sydney code sprint.

πŸ‡¦πŸ‡ΊAustralia mstrelan

Left a comment about the base class for the attribute class.

Also, let's update the description in \Drupal\views\Plugin\views\sort\SortPluginBase to mention attributes instead of annotations.

πŸ‡¦πŸ‡ΊAustralia mstrelan

A few type hints to fix. Probably worth a rebase to see if the tests still pass.

πŸ‡¦πŸ‡ΊAustralia mstrelan

I don't mind dropping named parameters, although it was advised again in another issue. I don't understand why you'd also swap back to double quotes when we general favour single quotes? Regardless, there seems to be a mix of both styles so no point changed back again now.

πŸ‡¦πŸ‡ΊAustralia mstrelan

Postponed on πŸ“Œ Convert ViewsField plugin discovery to attributes Needs review which has the required changes to ViewsHandlerManager.

πŸ‡¦πŸ‡ΊAustralia mstrelan

Postponed on πŸ“Œ Convert ViewsField plugin discovery to attributes Needs review which has the required changes to ViewsHandlerManager.

πŸ‡¦πŸ‡ΊAustralia mstrelan

Turns out we don't need ViewsHandlerAttributeBase, but we will still need the changes to ViewsHandlerManager, so this is still postponed.

πŸ‡¦πŸ‡ΊAustralia mstrelan

Thanks @alexpott I wondered about that and have removed it now.

πŸ‡¦πŸ‡ΊAustralia mstrelan

I've used CKEditor Inspector to compare the model of a working media with caption with what I'm getting with the patch.

Working:

<drupalMedia drupalMediaEntityType="media" drupalMediaEntityUuid="..." drupalMediaIsImage="true" drupalMediaType="image">
  <caption>Test caption</caption>
</drupalMedia>

Not working:

<drupalMedia drupalMediaCaption="Test caption" drupalMediaEntityType="media" drupalMediaEntityUuid="..." drupalMediaIsImage="true" drupalMediaType="image">
</drupalMedia>

I believe the issue is that the upcast converter is doing a simple attributeToAttribute conversion whereas it needs special handling for the caption. I guess the DrupalMediaCaptionEditing should take care of that but it just needs to be triggered some how.

πŸ‡¦πŸ‡ΊAustralia mstrelan

I was able to get the data-caption attribute inserted in the source by adding it in the this.attrs and this.converterAttributes in the constructor of the DrupalMediaEditing plugin thanks to @rikki_iki but it isn't editable, and pressing the "Toggle caption on" button adds an extra caption, overriding the original.

Attaching a patch to demonstrate, and a screencast below.

πŸ‡¦πŸ‡ΊAustralia mstrelan

We were expecting only int constant values already, so I don't think that would affect anyone?

Well then we should add the param type to all the other functions as well.

But it's possible that someone was passing '1' instead of 1 (or the const) and that will now break if the calling class has strict types enabled. Maybe that's enough of an edge case that we can get away with it.

πŸ‡¦πŸ‡ΊAustralia mstrelan

Had the wrong key in that patch, new one should be good.

πŸ‡¦πŸ‡ΊAustralia mstrelan

Attaching the patch this time 🀦

Production build https://api.contrib.social 0.62.1