Add type declaration to untyped scalar params on interfaces

we can safely add missing param types to classes and interfaces.

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

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.

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

mstrelan → changed the visibility of the branch 3436327-string-type-rector to hidden.

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

Fixed the broken tests, and everything is clear...I think this is ready for review!

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.

Wow! 🤩 Super impressive. Lots of changes to review tho. Would it be helpful to break this down into bite-sized chunks?

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.

I think if there are tricky/non-scriptable things we should split them off, but for anything scriptable and straightforward it's probably OK to do one big bang here. However, I have not actually tried to review the MR yet!

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.