[MigrationCreatorPatch-Issue-24907] Reference a variable of the insta…

[chrishalbert]

The purpose of this PR is to make a minor enhancement backwards compatible.

The firePostCreateHooks() is protected and in prior version accepts no arguments. For any applications or libraries using the code, we'll see those fail.

To fix the problem, I'm suggesting we assign the $table name to a property of the instance so that later the class can use that in the firePostCreateHooks method.

I was going to write a test, but saw this method which verifies existing functionality is unaffected: testBasicCreateMethodCallsPostCreateHooks

Issue Reference: #24907

[Miguel-Serejo]

For reference, the change happened in #24621. At the time, no concerns were raised, although it was technically a breaking change.
This PR essentially reverts that change and implements the same functionality in an alternate manner.

Changing this back now will fix old extensions of the class while keeping the new functionality for hooks, while breaking newer (about a month old at time of writing) extensions.

[chrishalbert]

I thought extra params wouldn't actually break the code. Does this change in 7.1 ?

[Miguel-Serejo]

The break would be the same as the break you've experienced, only the other way around.
Here's an 3v4l: https://3v4l.org/rPaov

Apparently HHVM doesn't have a problem with it, but that's mostly irrelevant.

To clarify, I'm merely pointing out that while this is a fix for a previous break, it will also break new code written in the past month or so. I think the original break should be fixed, but a note should be made about the new break.

[chrishalbert]

Oh, I understand what you are saying. Yes, if any one is subclassing and overriding that method, it will break their code. Good point.

How would you suggest a note be made about the new break?

[Miguel-Serejo]

Good question. A message in the update notes should suffice. Something along the lines of
** If you're overriding the firePostCreateHooks in your own extension of the MigrationCreator class, access the $path property of the class instead of declaring it as a method argument **

I'm not great at making documentation, but I sure like having it there.

[taylorotwell]

I am closing this pull request because it lacks sufficient explanation, tests, or both. It is difficult for us to merge pull requests without these things because the change may introduce breaking changes to the framework.

Feel free to re-submit your change with a thorough explanation of the feature and tests - integration tests are preferred over unit tests. Please include it's benefit to end users; the reasons it does not break any existing features; how it makes building web applications easier, etc.

Thanks!