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

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

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...

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

Re: #32 I think the return value from the DB is interesting but not instructive on what we should do here. Entity types e.g. user choose the format of their primary key/ID and if it's not changeable, we should typehint the value to be what it's intended to be. The fact it comes out of the DB query as a string (or maybe not!) is something to be normalized in the entity's code.

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 will simplify the interface and makes it easier to enforce strict typing ( https://www.drupal.org/project/drupal/issues/3050720#comment-13562649 🌱 [Meta] Implement strict typing in existing code Active )
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.

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.

It seems the overarching question is one of storage vs. runtime code contracts. I kind of think about this in the same mindset as bundle classes were introduced. If you have a field on an entity that represents a commerce_price item, for instance, you can introduce a getter that will return ?Price. It's otherwise only accessible by doing something like new Price($entity->field->first->value), which is silly boilerplate and fragile.

Similarly, if the ID of an entity has a field (and, by extension, database storage) property schema that is designed to always return an integer, then it's only appropriate for the entity class to return it as such, regardless of the underlying db handling.

Also by way of example, I recently worked on JSON data storage which introduces similar wrinkles - you can choose at a low level to cast return values as strings, or retrieve them in the native type they're stored in the JSON document, but how and how reliably that is done is a matter of database implementation specifics. You would still want to assert and return typed values in userland code for maximum type safety and portability across drivers.

Furthermore, the user entity has an even more compelling use case here, which is that 0 (zero) is a valid special-case return value for the anonymous user. Thus strict typing helps make it even clearer that !$user is unsafe because it matches both a null case and the anonymous user. While you could still write that bad code, ?string for a return type is non-specific and doesn't move us forward. ?int is at least true in a very specific way - and for methods that should always expect a user (which should be almost always the case) then it can be tightened to only int with no inheritance issues.

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.

Re: #40, to put a finer point on it, this kind of issue is complementary to PHPstan/static analysis initiatives in that it makes the code better reflect reality and support meaningful static analysis of custom code e.g. UID comparison.

Also linking a long-lived but important UID 1 issue: 📌 Add a container parameter that can remove the special behavior of UID#1 Fixed

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 🌱 [META] Stronger Typing for Entity API Active though it also falls under 🌱 [META] Update PHPDocs to reflect reality Active to fix the Docs.

Removing framework manager tag

Leaving as Needs Work to process through the MR comments.

Woohoo! Marking this as a task, not a bug. I have some sponsored dev time for core these days so I can loop around on this.

This tag is for https://github.com/phpstan/phpstan-strict-rules, which I don't believe is the intent behind adding this tag on this issue.