🇬🇧United Kingdom @andrew.farquharson

Hi @james.williams @jhedstrom @nerdstein @Steven Jones

Any chance a maintainer can close this and give credit? Thanks.

@ selwynpolit → The link is very helpful. It is a very comprehensive resource for PHPUNIT testing.

Okay, so is there a reason not to use 'http://localhost'?

@solideogloria Okay, your re-edit is fine by me. If you get "https://web" to work with any DDEV project name, even better. I could not. As far as i understand "http://web" is short and simple and will work with any and every DDEV project name?

I believe that is because the PHP container running the tests

The "SIMPLETEST_BASE_URL" for DDEV should be "http://web". "https://web" does not work.

DDEV PHPUNIT setup for Drupal

The kernel tests error free on my local dev: 1 test and 1 assertion.

I have also commited the .gitlab standard drupal template to the root of the projbect, to test my code using a gitlab pipeline.

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

I have applied the one liner fix at #18. I have not added any test because I do not know how a fresh installation using the UI is possible in, say, a kernel test. Unless someone knows how to create such a test, please review and test the fix manually.

To see how the issue is isolated to installations via the UI, try a site install using drush and note that the issue does not arise.

The proposed fix at #18 seems the best solution. It fixes the original issue as strictly described. I have tested site instalaltion using drush site:install (Drupal 11.x) command. The bug does not occur. I believe that fix #18 since it targets the file that controls site installation done via the Drupal UI. Fix at #4 is far more general in its action and could have side-effects.

@smustgrave I am not sure how to test the installation via the Drupal UI? Is it possible to create a test that tests the equivalent of a Drupal UI installation? If a test is not feasible, it is simple to re-create the issue using the summary and testing manually.

There is a more general issue, possibly, as described at #22. But that should be dealt with as a new issue.

Merge request !5634 has patch #4 applied.

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

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

Hi @Manuel Garcia, I am not sure if the gitlab configuration just needs to work for Drupal 10 and if not which versions of Drupal core the tests need to be tested on.. Also does the gitlab configuration it the .gitlab-ci.yml just need to be compatible with the 2.1.x branch or with other branches also?

Using setup to run gitlab tests locally I have run the PHPUnit tests on this issue branch: ddev phpunit. It reports OK (1 test, 29 assertions)

@Manuel Garcia, I have reviewed your most recent commits and I have inspected the output of the pipelines after your commits. I see that you have fixed the tests and all the assertions in the functional test completed. The test is fixed and the assertions are passing

I have not yet fully reviewed the change to the pipeline configuration. But i agree that the configuration needs to be tuned and I believe that the relaxation rule on the static functions should be relaxed to suppress warnings.

Since the test is now working properly I believe this issue is now fixed. But if there are more tasks to perform please I will try to help.

Hi @Manuel Garcia, Thank you for fixing the code! I am pleased that you found the source of the test error: I was struggling with the test. I will review the PHPStan config soon.

andrew.farquharson → created an issue.

Hi @djroshi, I think that Chetan 11's MR is preferable to your patch. He seems to be fixing the error without removing the serialization of the data which is not stated in the issue description. However I found an if block which seems redundant and so I have removed it.

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

1. First i copied the gitlab-ci.yml file created in issue #3406407 and committed it to this issue branch
2. I then fixed the test module compatibility with Drupal 10
3. That got the PHPUnit tests to complete successfully.
4. There were a few warnings in the eslint and php code sniffer tests that I fixed in further commits.
5. All the tests then ran successfully

First error-free pipeline

Not sure about the best ticket management but i think that if this passes review it could close issue #3406407 too.

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

@Manuel Ah, that makes sense.

1. I add a .gitlab-ci.yml file to the project root using the contents of the Drupal Association's template.gitlab-ci.yml.
2. Committed the file to the issue branch
3. On commit the pipeline was triggered automatically and ran the tests
4. The PHPUnit tests terminated with one failure.

I think a new issue should be created to fix the failing test.

Further investigation of the test failure can be carried out under this ticket to make sure that the failure is due to a test error: that the tests have reported the error correctly. That is proof that the pipleline is working correctly.

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

Thank you Manuel!

Hi @manuelgarcia. Thank you for closing the ticket. I enabled the Gitlab CI because it seemed otherwise my MR would not be tested. Was not sure how to set it up true! Thank you for the link: need to read up on it. I thought i would get credit for noticing the discrepancy between the core drupal versions between composer.json and the info.yml. Since my MR's have not been used I am not sure how the commit was incorporated? Did you create an ad hoc commit? I thought it was reasonable to fix that under the 'malformed composer.json' description of this issue. So I expected to receive credit. A bit discouraging otherwise..

@Manuel Garcia, Thank you for committing the patch, but there is an error in the composer.json that remains. That error is fixed in MR!4. The error is the drupal/core version which does not match the version in the .info.yml. That could cause problems. I provide a link to these problems in the docs in the description of this issue, which I edited.

Hi @djroshi, As i understand it others can review your patch and the merge request of chetan11. They can apply their own patch or make commits to the issue fork/ branch. They can create their own branch also. Others can then review/ test all patches and branches.

@djroshi This is not ideal. #23 Issue 3404802 has status 'Needs review'. That issue should be marked fixed and then closed before it is applied to other issues IMO. I will try to review it shortly. If others could do the same before trying to resolve this issue..

I am changing the version from 9.1.0-alpha1 to 9.1.x-dev because it seems that 9.1.0-alpha1 should remain compatible with D8 and D9. Whereas there is substantial work required in addition to D10 compatibility fixes to get version 9.1.x-dev in a state to cut a 9.1.0-alpha2 version from it.

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

I am changing this to the development branch.

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

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

andrew.farquharson → created an issue.

andrew.farquharson → created an issue.

@poker10 i regard this as a waste of time now. Good luck.

@poker10 I disagree. Others may agree. Good luck.

@poker10

(e.g. the checkbox is unchecked)

There are, in fact, three checkboxes which each is deselected while the other 2 are selected will trigger the error. →

The errors only occurs when using PHP 8.1 with Drupal 7.x. The fix I have applied is one that is used in PHP applications that rely on PHP 8.1, across the board. e.g. Laravel. Every previous version of PHP does not expose any faults in the code, color module, bartik or anywhere else. So I would say this is not a color module issue so much as a PHP 8.1 issue. I have applied a PHP 8.1 fix. It is extremely simple and effective, though I have not tested all the possible deselect permutations yet with the fix, only without the fix: they all trigger the same error.

@poker10 Hmm. I will look into this. I believe I have made the configuration form work when it was broken. I am not sure that following strict principles to prevent 'hiding' problems is necessary and thought I was being pragmatic! D7 has limited lifespan and I need to use it in production. I am sharing my own pragmmatic fixes as there may be others doing what I am. The root cause I believe is D7 with PHP 8.1. The error does not seem to appear with earlier version. You fix might be better but not sure its worth the extra effort? Strongarm has been superseded in D8 onwards.

andrew.farquharson → created an issue.

@poker10 I just did a drush site-install using standard profile. I have switched to the 7.x branch. I deleted all recent log messages. Then deselected the logo setting for bartik and saved the configuration. Then went to reports> recent log messages. The php typeerror was the most recent entry and also the only error.

@poker10 Hi, yes the other step was to save the configuration of bartik after deselecting the logo setting. I am not sure if you assumed that step or did exactly as i stated? If you could try it and perhaps deselect a few more of the settings, saving the configuration and re-checking the recent errors, please. I will try to reproduce the error once again if you cannot. Thanks.

HI @poker10, I tested on a fresh cloned installation. I unticked the logo setting for the default bartik theme https://www.drupal.org/files/issues/2023-11-17/clean-install-bartik-theme.png → . That triggered an error in recent log messages. Recent log messages → Two errors were triggered in the system.admin.inc file. First of 2 errors → The error was due to null argument for the array in in_array() functions Error message details →

This is a known PHP 8.1 issue. Fix for in_array function issue

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

@anairamzap I think that the method should be left now it is done. The README could be edited to say:

1. The module is only intended to be used within a limited set of configurations.
2. More unusual and complex configurations may not work
3. Using the module outside the recommended configurations may result in errors
4. Such errors should be tested against a recommended configuration
5. Other errors will not be supported

To test the patches I recommend installing the page manager module using composer, run a fresh site installation using the standard profile, enable the page_manager and page_manager_ui modules.

At /admin/structure/page_manager you will find the page automatically installed from the config/install folder of the module when the module is enabled.

Create a variant for the page and save it. Then click on the variant label then click on the 'General' tab. You can then edit the variant label and the page title. Then save/ update the edited values.

These steps can be created for the patched version of the page manager module.

The test can be repeated on several different drupal core versions of drupal that the module is compatible with.

The page manager module when enabled inserts a page from its config/install directory. That page applies to the /node/{node} path. But to change the layout of /node/{node} pages you have to add a variant. Then you can add blocks to the variant. Variants store page title and label values on creation. The label can be edited after creation but the page title cannot be edited after creation.

The same is true of any page that users create after installing the module once they have added at least one variant to the page. Pages do not store page title and label values. Only variants do. So this issue only affects page variants.

Hi @catch I have added comments.

 // Fixes a console error message generated in Firefox only
 // @see https://www.drupal.org/project/drupal/issues/3351600

I can edit them if they are not clear enough.

I have looked through the gitlab history of the file.module file.

The 2 lines of code that are now the subject of this issue seem to have first appeared in
commit file.module

The commit with SHA 988abc27f8b2f1a05e42932a39954dec4f18c09a was by Nathaniel Catchpole on 21 July 2013. In the description he refers to issue 2045189 →

So it seems that the source of the 2 $url_options lines is patch by jlindsey15 →

@smustgrave and @longwave I recommend a recursive search through the core folder. Search on '$url_options['language']' and you get 7 occurrences in 7 different files including file.module. Search on '$url_options = ['absolute' => TRUE];' and you get exactly 7 occurrences in the same 7 files. Both these lines are entirely redundant I believe and should be deleted from all 7 of the files (14 lines in total).

Perhaps create related tickets for the other core modules in which the lines appear.

Hi kim.pepper and larowlan,
I have searched core codebase recursively for the line,

$url_options = ['absolute' => TRUE];

The line is redundant and the variable is not used in the file module. So I believe it can be safely removed.

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

@drupgirl Can you please share your composer.json

The Unit test does not seem necessary. Manual testing seems the simplest method. Just manually edit the user.role.authenticated.yml file and set 'permissions: NULL'. Then access the admin permissions form and refer to 'recent log messages' under admin/reports and look for the InvalidArgument Exception message. No errors should be generated.

I have tested patch quiz_d10-3356316-4.patch on a Drupal 10 site. I enabled the short question and multiple choice question types, created a test quiz, added questions, used the randomisation to change the question order and undertaken the quiz as a quiz user. I generated two sets of quiz results. No errors were generated on-screen or reported under Reports > recent log messages.

1. I started out by creating a Drupal 10.0.9 site using DDEV-local.
2. Then followed this documentation: http://www.yuseferi.com/en/blog/How-apply-patches-Drupal-8-Composer
3. However, in addition it was necessary to install this plugin: https://github.com/mglaman/composer-drupal-lenient
4. So i ran this command: ddev composer config minimum-stability dev
5. Then ddev composer require mglaman/composer-drupal-lenient
6. Then composer config --merge --json extra.drupal-lenient.allowed-list '["drupal/quiz"]'
7. Then this command worked: ddev composer require -W 'drupal/quiz:6.x-dev@dev'
8. If you have not done so already, you need to install the cweagans/composer-patches plugin also and install the above patch.

I have successfully tested patch #32 on the Drupal 10 dev after installing the module and applying the Drupal Lenient plugin.

@Wim Leers The error does not appear in Chrome. I see that the nightwatch.conf.js file in core only sets up a chrome environment. But not currently Firefox or Safari. To test the error using Nightwatch I would have to add an environment for these other browsers.

@smustgrave Is that something that is permissible? Or is the core nightwatch config restricted to Chrome only? If Chrome only then it seems as @Wim Leers says creatoin of an automated test for this issue is not possible.

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

@smustgrave, thank you for posting in Slack. Although i have been testing with a contributed module to make ckeditor5 to load in a modal, it may not be best practice in core tests to use somewhat random contrib modules as test dependencies?. I am thinking to test this modal functionality in core, it requires a more 'native' way of loading a ckeditor5 field(s) in a modal? So I am wondering if there is some Nightwatch technique that can load a specific form in a modal?

@smustgrave Yes I need to re-read the original description of this issue. I can install the add_content_modal:^1.0 module, make it a test dependency. I will also have a look at nightwatch- see if it can supply any testing needs.

@smustgrave

This is one example core test that seems to cover everything I could possibly create a test for:

core/modules/ckeditor5/tests/src/FunctionalJavascript/CKEditor5Test.php

It not only creates a text field for a node and activates ckeditor5 for that field, it inserts text into the text field using every possible button in the toolbar of the ckeditor and saves the result..

And it is one of 20 'FunctionalJavascript' tests. I am assuming that all these tests are run when anyone runs automated testing? Are you expecting in addition to all such tests me to write a custom one? If so, what do i need to test that is not being tested already? Or are these core tests not triggered when choosing 'Add test / re-test'?

If they are triggered then for overall functionality of the ckeditor5 module then these tests seem comprehensive. The error I am trying to fix though triggering an error does not seem to break any functionality so by in addition, manually viewing the browser's console, it seems to cover the bases?

@Berdir @akalam I can start scaffolding a new plugin type for the media type module. The plugin type will need an interface so maybe this ticket can be closed as complete since the interface seems complete and i think it can be now be re-used in the plugin type (if it is agreed that decorator solution is not the right path to take).

@akalam do you want to create a child ticket and close this one?

@Berdir, do you have any views on this?

@akalam Yes, it seems that currently the media library extend module code is not using the decorator pattern. They are using this technique:

if ($state->get('media_library_extend') !== '1') {
      return parent::buildLibraryContent($state);

}

That workaround seems like a poor substitute for proper implementation of the decorator pattern: using an interface.

Hi @smustgrave,
By 'test coverage' do you meaning running the existing tests or writing new ones? The existing tests cover the core functionality, i think, so if creating new tests, what operations would they need to cover over and above core functionality?

@akalam, The media library extend module is using a service decorator to extend the service. I agree that the decorator pattern should really be using an interface and this interface seems to be the answer.

Are you suggesting that by using an interface in the media library module that the media library extend module will be made unnecessary?

Are you suggesting that developers will be able to implement their own custom modules and decorate the MediaLibraryUiBuilder class/ service?

Where do the plugins mentioned by Berdir fit in with your vision?

Hi akalam, I have read through the code. It makes sense. Looks good.

I tested MR !3972 using Tugboat:

1. logged in as admin
2. enabled media and media library modules
3. Added media field and enabled all media types to Basic page
4. Created a basic page with an image added to the media field
5. The page displayed okay with no errors

If you can use DDEV-LOCAL, here are the command line steps if once you have cloned the repository, forked the issue and checked out the branch:

ddev config --project-type=drupal10
ddev start
composer install
composer require drush/drush
ddev drush site:install --account-name=admin --account-pass=admin -y
ddev launch
ddev composer require 'drupal/add_content_modal:^1.0'
ddev drush en add_content_modal
ddev composer require 'drupal/devel:^5.0'
ddev drush generate-content 20
ddev drush cr

You will have to go to disable aggregation of css and js, clear all caches
Configure the add_content_modal: enable it for at least one content type

Merge request !3951 tested in Firefox 113.0

Merge request !3951 tested in Safari 16.2

#22 Merge request !3951 tested in Chrome