UserInterface::id(), AccountInterface::id() should return int to match typehints

@bradjones1: Thank yo for working on this issue. I have talked to user @catch on Slack about this issue. His suggestion is to update the docblock of AccountInterface::id(), so that it is saying that the return value is a string value instead of an integer value. User @catch is one of the core framework managers and one of the core release managers. My suggestion is to go with his suggestion. I will help this issue by reviewing the MR.

As #3324733: UserInterface::id has illegal return type hint in PHPDoc has revealed PHPStan [Strict] gets upset about the incompatibility of \Drupal\Core\Entity\EntityInterface::id and \Drupal\Core\Session\AccountInterface::id, particularly the nullability part:

Return type (int|null) of method \Drupal\user\Entity\User::id() should be covariant with return type (int) of method Drupal\Core\Session\AccountInterface::id()

Whether User::id returns a real integer, not a string-int, is one issue.

However it can return null, such as on creation.

\Drupal\user\Entity\User::create(['name' => 'foo'])->id() === NULL;

Given User::id() in the MR can still return NULL, are we considering updating the docs for AccountInterface::id()?

I think $id !== NULL ? (int) $id : NULL; is the best option. I think it's better to avoid using is_null() and prefer instead to compare to the literal NULL.

Before any more work is done here, maybe @catch or another core framework manager should chime in and decide if we want an integer or string return type, and if changing this should be done in a minor/major version with a deprecation path. So far I have only seen suggestions, no decisions.

Personally, I think an integer makes the most sense since it's numeric. The database fields are also integers.

+1 for standardizing on int, whenever / however that's possible. 😅 Would love to get formal decision from framework and/or release managers before we proceed. I'll ping some folks in Slack to try to get a decision. Meanwhile, tagging to be smashed.

Left some thoughts, leaving the Needs FM tag because other FM's may feel differently

The (content) entity system is not strictly typed, this has been discussed before but I don't see that this will change. The database returns all values as strings (and even if that would change, the database doesn't have a boolean type for example) and field API's are designed to use those values as-is when accessed without looking up definitions/types.

There are ways to get the type, jsonapi/rest does that that to a certain degree to get correct types, but that adds performance costs.

It's also not possible to really enforce this on UserInterface or AccountInterface with a return type because UserInterface extends EntityInterface and that can definitely return an integer or string depending on the entity type. So adding a type on UserInterface::id() would prevent us from ever adding the correct and wider return types on EntityInterface::id().

I'm not saying that we shouldn't improve this, but it's a far wider problem than just UserInterface::id() and my recommendation is in general to _not_ use strict type comparisons when working with content entities. It's now how modern PHP is generally written, but it reflects how the system works and it would require major changes to change that.

Further proof, if any more were needed, that #berdir-is-always-right. 😂 Was tempted to start an issue tag for that, sorta surprised one doesn't already seem to exist. 😉

I think there's a typo in the last sentence (s/now/not/), but otherwise, #25 is a fairly convincing argument that we can't actually fix this here like this. Leaning towards won't fix, unless we can come up with a new scope / title / summary for a way to improve this that doesn't have all the problems @Berdir just spelled out...

So adding a type on UserInterface::id() would prevent us from ever adding the correct and wider return types on EntityInterface::id().

An extended interface can return a more narrow list of types.

EntityInterface::id(): string|int;
UserInterface::id(): int; 

Is acceptable.

is in general to _not_ use strict type comparisons when working with content entities.

We have a coding standard for declare(strict_types=1); now and more and more code is using it. Also with us using phpstan as part of the contrib gitlab_templates this is already extremely painful to deal with.

If were not going to have UserInterface::id() return an integer as its API specifies, than we should be looking at changing its return type to be a string so that tooling can properly work with it.

I agree with @cmlara. Proper programming practice is to accept the broadest type possible for parameters, and return the most specific type possible for return value types. If you know it will always be ?int, even when the parent is string|int|null, it's okay to return ?int.

Currently, PHPCS is confused when using the return values of the UserInterface::id() function, because it says a string value will be returned.

> An extended interface can return a more narrow list of types.

Noted, I always mix that up, it's the other way round that doesn't work.

I agree about strict typing in theory. My point is that trying to force code working with API's that aren't using strict types to be strictly typed is in my opinion going to cause more bugs than avoid them. That's certainly been my experience, I've had to fix plenty of bugs that expected certain types which weren't actually guaranteed over the years.

Either way, the patch definitely needs work, per review from @larowlan. All those places definitely can be NULL and that needs to be documented, and the documentation needs to go on UserInterface and AccountInterface, not User.

Removing rescope as this still needs a framework manager decisions since the blocker from #25 doesn't exist.

Updated IS to show the options that are awaiting a framework manager decision.

An anecdote,

Two years ago on a large project I implemented the following trait on the bundle classes of Node, User, Media, and others. For which we have bundle classes of all bundle types. It coerces IDs to integers, so we may confidently set our return types and documentation to int|null and have stricter static analysis.

trait IntIdTrait {

  public function id(): ?int {
    $id = parent::id();
    return $id !== NULL ? (int) $id : NULL;
  }

}

With this change, I would not consider the effort required to get things working arduous. I recall #3318453: Access to TFA page is denied depending on entity/database internals may have been the only contrib issue raised.

Hopefully this improves confidence merging proposed changes.

The database returns all values as strings...

Apparently this isn't always the case either, and is (according to issue statements) not the native default for any of the PDO database layers we work with currently.

Return values for the UID field will only be strings if the database PDO options do not override \PDO::ATTR_STRINGIFY_FETCHES. I concede it is fair to believe this would be exceedingly rare, and may have unexpected adverse impacts including breaking sites however it is (at least in the MySQL driver) not enforced as a mandatory option.

Related Context:
#3224245: Open MySQL connection using \PDO::ATTR_STRINGIFY_FETCHES

Better fix for the DB integer stuff. This is a MYSQL only change. We need to use \PDO::ATTR_STRINGIFY_FETCHES => TRUE like the other drivers. At some point we can change core to not do this on all the DB drivers but that is for later

https://git.drupalcode.org/project/drupal/-/merge_requests/937/diffs?com...

EDIT: I missed that comments #12 and #13 mentioned this already in 2021.

According to docs it should add commented-out type-hints https://www.drupal.org/about/core/policies/core-change-policies/drupal-d...

I can't remember the exact slack conversation with @daffie but my general feeling is if we're going to start casting database results, we should open a meta issue and also do that for nodes, taxonomy, comments, menu link content etc. etc. since they all have the same problem for IDs. There'll then be other base fields that could also be more strictly typed etc. so this is a not a small one-off task but a significant project that needs to be coordinated across core entities.

But... I also think the docs should reflect reality - whether that means casting here as a one-off/first of the meta issues, or updating things to reflect reality then changing them again later, I don't have a strong opinion.

I strongly advocate for fixing this specific issue with a cast now because it breaks strict type enforcement, as mentioned in the OP. It's very frustrating to write several custom modules, run a bunch of tests in phpstan and everything checks out, and then when you actually use your site you start getting WSOD because the typehint in core was incorrect.

What about changing the return type/hint of EntityInterface::id() and all other ID parameters//returns to ?string and dropping int altogether as possible return type.

IDs are used to uniquely identify entities and not to do calculations with or anything.
Dropping int as possible type for entity IDs will simplify the interface and makes it easier to enforce strict typing (https://www.drupal.org/project/drupal/issues/3050720#comment-13562649)
It will still be possible to do things like auto increment (no issue at all when handled by the DB) and finding the next available (numeric) ID.

I think almost all existing code will continue to work; as already stated, the type of ID variables are already inconsistent.

What about changing the return type/hint of EntityInterface::id() and all other ID parameters//returns to ?string and dropping int altogether as possible return type.

I don't think that makes sense. We need to work towards strict typing with a straightforward and common-sense standard. It's not a good idea to standardize returning all integers stored in the database as strings.

We shouldn't have to be unsure if we can cast an ID to int and add one. If it's a string, users will likely and up being unsure if it's guaranteed to be numeric or not.

I can't remember the exact slack conversation with @daffie but my general feeling is if we're going to start casting database results, we should open a meta issue and also do that for nodes, taxonomy, comments, menu link content etc. etc. since they all have the same problem for IDs. There'll then be other base fields that could also be more strictly typed etc. so this is a not a small one-off task but a significant project that needs to be coordinated across core entities.

Ultimately, yes, Drupal Core does need an issue to handle casting. We will either forcibly hit this with the return typehints+strict_types initiatives, PHPStan initiative, or just PHP no longer allowing coercion on native method calls. This issue to be int or string will need casting one way or the other (as noted above regarding how the DB returns results as both as such we must cast no matter the decision)

Having witnessed how hard it has been to remove REQUEST_TIME from core which is a simple change the odds are that fixing these issues globally across core is likely to be single method or at most a single Interface at a time.

While the number of lines changed for this issue are small (at the moment) UID's are so widely used across the ecosystem we would likely always want this to be a 'single method' only change.

While the number of lines changed for this issue are small (at the moment) UID's are so widely used across the ecosystem we would likely always want this to be a 'single method' only change.

Hopefully, once impacted users change one method, they will see the upcoming pipeline of changes and change in bulk what they need to ahead of time, if possible.

For example, if someone is doing $user->id() === '1' in their code, and doing the same thing with other entities, they can make a change across their codebase to use == 1 instead, with the eventual possibility of using === 1.

I think #35 feels like the best approach here as follows:

• We start a meta issue for bringing stronger typing to the entity API
• We start a parallel meta for making the docs reflect reality

This issue would be a meta of the first one I assume, but we could split out docs work in the the later to improve the life of those using stricter phpstan rules.

Per SLACK https://drupal.slack.com/archives/C1BMUQ9U6/p1707856521514239

This issue will continue enforcing the strict integer return under #3441689: [META] Stronger Typing for Entity API though it also falls under #3441690: [META] Update PHPDocs to reflect reality to fix the Docs.

Removing framework manager tag

Leaving as Needs Work to process through the MR comments.