Model hooks

[wimski]

In the last couple of Laravel projects I did, I needed extra IDE help for models (translated properties, attachments, etc.). There currently is no way to generate custom information so I wrote a model hook system. Everyone can then easily extend the generation of model information. In order to make it actually useful, I changed some methods in the models command from protected to public.

[mfn]

I feel angsty opening up ModelsCommand suddenly being so public because it's so much spaghetti in there, but I like how non-obtrusive your approach is so I wouldn't see a reason why not.

Pretty great PR IMHO!

[mr-feek]

Context:

While working on https://github.com/psalm/psalm-plugin-laravel, I've desired the ability to "change the template that gets rendered" (akin to #942). I understand that would require a lot of refactoring, so I haven't gotten around to do it yet.

---

Feedback:

The verbiage run seems to me it may be a bit confusing, as the hook can only add information to be written, right? Shall we consider making this a bit more clear on the interface level? Thoughts?

---

Sidenote

Do we consider the API of ModelsCommand to be in a done state? I personally feel this class could be reworked a lot, and therefore would be hesitant to exposing it publicly.

[mfn]

@wimski could this PR solve #930 ?

[wimski]

@wimski could this PR solve #930 ?

@mfn this PR doesn't add functionality to the IDE helper. It only opens up some of the existing functionality so you can add information from sources the IDE helper doesn't know about on its own. So this could solve #930 if there is already functionality in the IDE helper to write/define custom tags (which I didn't look into). If not, then this PR won't help with that.

[mfn]

I thought about this again…

#945 (comment)

I personally feel this class could be reworked a lot, and therefore would be hesitant to exposing it publicly.

It matches my sentiment from #945 (review)

I feel angsty opening up ModelsCommand suddenly being so public because it's so much spaghetti in there

But currently it's a catch-22.

No one came forward with a clear plan to clean this up and I can imagine such a refactoring could be quite intrusive.

For example what I described in #756 is kind of critical to my current use of ide-helper (I do use ide-helper within a real Laravel project but also in one were individual components are used) and I had to jump through quite hacky hoops to get it working. Was like 80% duck typing and 20% relying on extending code.

No one came forward with a clear plan

That's kinda a point also, because I can surely come forward with a nice PR I feel is the "clean code of 2020" but what problem would it solve? Enabling things not possible before should be part of that vision.

---

Although the PR is quite unobtrusive technically, it could open can of worms in the current state hard to close, once the hooks "catch on". I'd err on the side of @mr-feek and question if it's the right to go ahead of this.

---

Apologies for all the meta babbling…

[GuidoHendriks]

What's the status on this? Would really appreciate this change.

[mfn]

The technical implementation here I think is nice and unobtrusive, except for the fact it opens up everything publicly. For a long time I was torn on this one and was leaning towards accepting it.

But the more I think about the implications, the more I'm worried.

I think the high level idea is great (providing a way of extending / intercepting ide-helpers functionality).

But as I already mentioned, there are so many smaller issues with the code base and then guaranteeing them all as part of the model hooks to work that way what previously was rather "internal details" worries me the most.

Alone the fact we've passing a full Symfony Command into the hook and thus making it the primary point of contract IMHO feels absolutely wrong:

interface ModelHookInterface
{
    public function run(ModelsCommand $command, Model $model): void;
}

---

I'm aware not every project on earth must fulfill the "clean architecture 2020 guidelines", let's be realistic. But open things up that way right now can cause a lot of issues for users of hooks when we want to improve / refactor the code base. That's why accepting the whole ModelsCommand is so dangerous.

I think more water should flow down the river first before agreeing on an implementation for such a topic.

---

@barryvdh how do you feel about this?

[mfn]

@barryvdh there was a similar PR opened recently which achieves the same as this one -> #1157

I gave technical arguments in the other PR why I prefer this would but seems we lost track of it, my last comment went unanswered :)

My TL;DR: If you are fine with opening the ide-helper for this, I'm fine too (and ppl really want to have this feature…)

[wimski]

@mfn I've rebased and fixed my branch and added an entry in the change log, so it's ready to go if you are 😉

[mfn]

Let's do it, 👍 from my side

Rolling over the ⚽ to @barryvdh 😏

[daniel-de-wit]

Would love to use this 👍

[mfn]

@barryvdh would you be fine with this hook?

[Miljoen]

This MR looks great to me :)

[wimski]

@barryvdh 👋

[caugner]

FYI @wimski @barryvdh This was a breaking change for psalm/plugin-laravel, which has a FakeModelsCommand that overwrites this method, see: psalm/psalm-plugin-laravel#137

[wimski]

@barryvdh I guess it should've been released under 10.0.0. I also didn't realize this would've been a breaking change, which it obviously is by changing the visibility of methods 😢

Maybe remove the following from the CHANGELOG.md:

[Next release](https://github.com/barryvdh/laravel-ide-helper/compare/vX.X.X...master)

You won't know the version number until you're ready to do a release, because it depends on the changes since the last release.

[mfn]

@caugner thanks for bringing this to attention!

Maybe remove the following from the CHANGELOG.md:

This line is fine IMHO, it always shows the diff from the last release to current master (it was broken wrong recently due to a mistake 🤷‍♀️

Breaking changes: unfortunate, I guess. No one thought about that. I mean, hardly anyone would assume the use ide-helper as an actual library though it's of course a valid use case. In this case it's slightly different. Or actually not.

We probably shouldn't roll back now, likely not worth it? And going forward, being more concise about this. Personally I don't have a problem pumping out more major versions in case we see a break somewhere, I'm not attached to version numbers or how they look.

[mfn]

Next breaking change incoming, see https://github.com/barryvdh/laravel-ide-helper/pull/1198/files#r606697817

[barryvdh]

I think it's a bit of an edge case and indeed not the intended/expected usage. But we could agree to bump minor versions for this, so plugins can specify minor versions.

[caugner]

I'm not sure how other comparable projects handle this, but one could argue that protected methods on non-final classes are part of the interface, even though it certainly is an edge case.

Since specifying minor versions is rather unusual (afaik, as only major versions are expected to contain breaking changes)
it would probably be best to bump the major version. psalm/plugin-laravel currently targets ^2.8.0, so a minor version bump (although semantically more correct; it's a feature, not a bugfix) would not make any difference for that case.

What would be reasons against a major version bump (and reverting the change in v2)?

PS: psalm/plugin-laravel seems to be the most widely used library that depends on barryvdh/laravel-ide-helper: https://packagist.org/packages/barryvdh/laravel-ide-helper/dependents?order_by=downloads

[barryvdh]

That all end-users need to upgrade their dependencies.

[mr-feek]

Making a public class method visibility change in a minor version will still have the potential to leave lots of users in a broken state. We released versions of psalm-plugin-laravel saying that we work with "barryvdh/laravel-ide-helper": "^2.8.0". So if anyone updates ide-helper without updating psalm-plugin-laravel, psalm-plugin-laravel will be broken.

The only real, backwards compatible solution here is to make a hotfix release reverting the changes, and then re-releasing the breaking changes in a major version.

IMO "model hooks" brings a ton of new functionality and increases the public interface of this package which in itself could warrant a major version upgrade.

---

That being said, I understand 100% that it was not expected for this class to be used outside the codebase. But we did because it brought great functionality to statically analyzing laravel projects and it wasn't marked as final 😄

My push is for a major version change, but I understand the rationale of being a bit reluctant to do so.