Use static analysis to detect new update functions, to reduce false positives in StagedDBUpdateValidator

I think adding nikic/php-parser to core has other good use cases too, such as parsing annotations or PHP 8 attributes. If there isn't a core issue already for adding this library to core, let's open one.

Isn't core already depending on nikic/php-parser?

This seems like it potentially is worthy to be tagged core-mvp, since it has such a big impact?

I think this is Major at minimum?

Can we convert this into a Plan and create multiple child Tasks?

Just to be clear

Currently, we just use simple hashing of the .install and .post_update.php functions to check for changes, but this can produce incorrect results if comments, whitespace, or runtime code like hook_requirements changes in those files.

I believe all of these could only produce false positives, determining there is an update when there is not. Not false negative, determining there is not an update when there is.

I think this case is much less critical because:

1. This means we are not going missthat a code update has a DB update and then allow it to run in cron
2. It seems very unlikely that we would determine security update does have a DB update when it does not and therefore not run it in cron. This is because security updates don't ship with unrelated fixes so we won't get
   

   comments, whitespace, or runtime code like hook_requirements changes in those files.

   if there weren't functional changes in those files

So this may be a reason for this not be major. I personally don't see why this would need to be core-mvp(unless everything we would ever want the system to be is core mvp😜). It doesn't seem changing this later would cause any API changes

I figured it was potentially core-mvp because it has significant security implications. Not because we missed something, but because we would prevent a security update from getting installed precisely because this is overly cautious…

Isn't core already depending on nikic/php-parser?

It seems to be in composer/Metapackage/CoreRecommended/composer.json but nowhere else core so I am not sure what this mean. If it is a direct core dependency shouldn't it be in `core/composer.json`.

does that mean we should make a core issue to add the dependency to core before we put work into using in our module. Assigning to @Wim Leers because maybe you know how this works

Turns out it's a dev-only dependency for phpunit. But this does seem like a low-risk external dependency.

I do think that Major is still accurate based on #5. I still think that reducing false positives on this front is something with indirect security implications.

However, after inspecting the code (\Drupal\package_manager\Validator\StagedDBUpdateValidator), I do see that we can be 99% confident that this would be an isolated change, without the need for API changes/BC breaks. Plus, it's marked @internal.

So let's explicitly postpone this to a later phase?

So let's explicitly postpone this to a later phase?

Since @tedbow didn't respond after a week, explicitly assigning to him.

Yes I think we can postpone this for reasons stated in #7

I think we could do this after the first core commit but maybe @Wim Leers disagrees

I agree 😊

So … is this a must-have for beta/stable, or a should-have for stable?

Marking Needs review to get @tedbow's response to #11.

I think for stable. Not sure how we should tag issues core-stable-blocker and core-beta-blocker?

Why not just stable blocker and beta blocker? The implication is that it's referring to core?

Oops! Was just testing it out :P

I noticed that 10.0.8 to 10.0.9 actually has a change to system_requirements that would flag has a DB update when it is not

#18 happened at #3351895-24: Add Drush command to allow running cron updates via console and by a separate user, for defense-in-depth.

crediting @FizCS3 for finding in testing

@phenaproxima mentioned token_get_all(). It looks REALLY promising: https://3v4l.org/SX0rT

Any sequence like this:

T_FUNCTION
T_WHITESPACE
T_STRING

we'd need the value of T_STRING for, and … we'd have a complete list! 🥳

re #18 we should manually test that this new method would not have the false positive for the DB updates that current method has

I few comments in the MR

Thanks for testing!

That looks amazing! 🤩

I am a bit curious if this is sufficiently fast for large codebases, but considering how little code is typically in *.install and *.post_update.php files, I'm not very concerned. Still, this might come back to haunt us at some point in the future in some extreme cases…