🇨🇦Canada @hussainweb

Even though this MR is not merged, I have put in a different fix in 🐛 Usernames with Spaces do not work Needs review , adapted the test here, and credited you there. Thanks!

Let's not make this more complex than it needs to be and we'll just remove the line, like others have 🐛 Drush 13 compatibility Fixed .

hussainweb → created an issue. See original summary → .

I pushed a change to the test removing some of the use cases. The usernames being tested are actual users. In this case, that user is not confirmed and the URL returned by the API is different than what we would expect (it is of the format /user/uid). We can test this as well but this test might break in the future when this user gets confirmed. So, let's remove it instead.

I just found 📌 Drupal.org profile link in display page is formatted incorrectly Needs review which is the same issue as this. I am brining in the test from that issue and crediting the author.

The upgrade status module reports additional issues with the tests. There are calls to `prophesize` that look like deprecated code but it isn't. Drupal's base UnitTest class provides the integration with prophecy-phpunit that is used to handle this deprecation.

This means that the only change required is in the info file.

@nicxvan, I have adapted your fix into a different MR. Can you take a look and review? Thanks!

Thanks, @nicxvan. You said you'd prefer the UID, but your example indicates a URL.

Also, what have you entered in the fields on the page you linked to? I am guessing you have entered the UID itself and that is why you created 🐛 Usernames with Spaces do not work Needs review . Is my understanding correct?

hussainweb → created an issue.

Thank you for the MR. Can you take a look at 📌 Decide if we should accept actual username or URL slugs Active and let me know your thoughts? We can fix it properly that way.

hussainweb → created an issue.

Merged. Will set up a new release soon.

I assume this worked. Please feel free to reopen if not. Right now, the module installs fine on Drupal 9.5 at least (and soon on Drupal 10).

I just committed a few changes related to timezones in 📌 Handle timezones Fixed and that might be tangentially related. Can you try this again?

Merged

A fresh test with Drupal 9 and 10 showed that timezones are not handled correctly. When saving a "Date only" field, the actual date written to the database is a day earlier (as I am in Eastern timezone and Drupal converts the date to UTC). For example, today is 4th Jumadil Ula and 16th November; saving this date will store 7 PM 15th November. The time is discarded and only 15th November is stored. On the next load of the edit page, it will convert 15th November to 3rd Jumadil Ula. Saving this will store 14th November and the drift repeats.

The problem is that the date form element we have handles the date conversion and it returns a string after formatting the timestamp with Drupal API. Of course, the timezone conversion happens here. Therefore, the date form element (TaarikhDate.php) should set the timezone before returning the formatted string.

Updated README with instructions.

We use DDEV to work on this module. The DDEV configuration is ignored from git for two reasons:

1. It's unnecessary.
2. It locks in the Drupal version.

Instead, we'll use a script to set up the development environment. Fortunately, [ddev-drupal-contrib](https://github.com/ddev/ddev-drupal-contrib) makes this very easy for us. The steps mentioned there are scripted in `./setup_dev.sh` and some more. The script also lets you specify the Drupal version to use (corresponds to DDEV recipes: `drupal8`, `drupal9`, and `drupal10` as of right now). It also copies a settings.local.php and modifies settings.php to have it take effect.

Usage:

./setup_dev.sh drupal9

This will set up a DDEV site at the address https://taarikh-drupal9.ddev.site (if you have cloned this repo in a directory called `taarikh`). Now you can install the site and happy testing.

hussainweb → created an issue.

This has been fixed and tested for a while on PHP 8. Marking this as fixed.

hussainweb → created an issue.

It turned out to be simpler than I thought. Merging and fixed.

hussainweb → created an issue.

The phpunit task is still failing and I'll do this in a follow-up (even phpstan). For now, this is merged.

There is an error: 'Could not open input file: vendor/bin/phpstan'. This seems to be a problem with how Gitlab CI is configured in templates. There is some information in https://www.drupal.org/project/gitlab_templates/issues/3395419 → but it seems that this was overlooked. Possible fix is that we include phpstan as a dev dependency which should be done in a different issue. For now, we are good to ignore this.

Thanks! I'll test it today and commit.

hussainweb → created an issue.

Fixed and all jobs in the Gitlab CI pipeline now pass.

hussainweb → created an issue.

hussainweb → created an issue.

For the second error, it is a common Drupal practice and the advice is to ignore it as per https://www.drupal.org/docs/develop/development-tools/phpstan/handling-u... → . The other two errors are fixed.

The module contains a phpcs.xml.dist from here: https://git.drupalcode.org/project/examples/blob/HEAD/phpcs.xml.dist.

As such, the current version of the file is very similar to the one in the above link (except the sniff referred to in the error message). While we can update the file, I think it is more productive to just remove the file and let Gitlab CI pull in the default one. We can bring this one back if we need to customize anything.

hussainweb → created an issue.

hussainweb → created an issue.

As it turns out, the file being reported is drupalci.yml and we don't need that anymore. Deleting it...

hussainweb → created an issue.

Committed and pushed. Thanks.

This seems to be a recent change to TypedData interface as I have not seen this before. Thanks for the patch. I can see that the same pattern in a variety of classes in Drupal core.

I released a new version of a dependency package that solves the problem for `core-recommended`. I then tested this on Simplytest and this module is installable on Drupal 10. No further change was needed in the module itself.

I tried this again today and this still needs work. The module is installable on Drupal 10 if you use `Drupal/core` composer package. But not installable if you use `Drupal/core-recommended`.

Thanks for your work but I want to make a release today and so I am just going to push a simple fix for now. We can consider autowiring later. If you're interested in taking this further, please create an issue.

Committed and pushed. Thanks!

Thanks, @Shreya_th. I am afraid this doesn't work either. You are reading the expression into a variable and the warning is generated at that point. By the time you check for `!empty`, the warning is already generated.

The fix is to use the null-coalescing operator.

Committed, thanks!

Patch in #4 is good. I am afraid that MR !7 is targeting the wrong branch.

IMHO, we can just fix the error here. Autowiring and testing it is a larger problem we can worry about later. I am not sure that TypedData even supports injection this way. Of course, I wrote this ages ago when autowiring was not implemented in Drupal core, so things must have changed.

The fix is already there in your MR. You just need to remove the injection bits and we can create another issue to inject the service properly.

@abhishek_virasat, I think you're trying to rely on autowiring for this to work, but autowiring is not enabled. Did you maybe forget to commit that change? Or am I missing something here?

@jagraj_singh_gill, your change is good in theory but we should return an empty string rather than `NULL`. This value gets used within Drupal and I am worried this NULL will throw other errors. We only hit this case when the bio is empty on Drupal.org. In that case, empty string is good enough. Please make this change and I can commit.

Thanks for the fixes.

@Shreya_th, I see your patch and MR are different. The patch has accidentally changed the indentation.

+++ b/src/DOComputedFields.php
@@ -33,42 +33,59 @@ class DOComputedFields extends TypedData {
+    return is_array($userInformation->field_bio) && isset($userInformation->field_bio[0]['value']) ? $userInformation->field_bio[0]['value'] : 'N/A';

I see you are treating the `$userInformation` as a Drupal field value. It is not. It is a simple PHP class and the `field_bio` (magic) property is not an array (unless it is empty). So the above code will always return "N/A". Yes, it won't throw an error.

In your MR, on the other hand, you are using a function called `get` but that function doesn't exist on the `User` class at all (nor on the base class).

hussainweb → created an issue.

I accidentally created the same issue for 2.x at 📌 Decide if we should accept actual username or URL slugs Active . Closing this one.

hussainweb → created an issue. See original summary → .

hussainweb → created an issue.

@apaderno, like I said in #3, the patch/commit is not visible here but there is still something that needs to be tested, and hence it is set to "Needs Review". From the documentation → :

Note that if a patch is not required in order to complete the idea described in the issue, then it is the idea itself that needs review.

I am interpreting that as there is something here that needs to be reviewed and it is fine for the field to be set that way. I understand it might break your workflow and I don't know of any solution for that.

Just removing the $logger property on the class is sufficient. That property is made available by LoggerAwareTrait anyway and the error was due to different types (it needed to be `?LoggerInterface` whereas it was missing and assumed `mixed` in the module's class).

We can still set to the property in the constructor because LoggerInterface is compatible with Drupal's `\Drupal\Core\Logger\LoggerChannelInterface`.

hussainweb → created an issue.

hussainweb → created an issue.

@abhishek_virasat, did you try recently or are you saying that you were able to install this before I created this issue? I have already fixed this issue but for some reason, the commit doesn't appear here.

hussainweb → created an issue.

Shriaas → credited hussainweb → .

volkswagenchick → credited hussainweb → .

I can't find the references I meant to update. Weird... Meanwhile, I'll clean it up a bit.

hussainweb → created an issue.

hussainweb → created an issue.

hussainweb → created an issue.

hussainweb → created an issue.