Improve title handling, especially for example scenarios

[uuf6429]

Fixes #270

---

Remaining points

• Cover changes with tests
• What about interfaces? Changing them constitutes a BC break. (getTitle is defined in KeywordNodeInterface - there's also ScenarioLikeInterface and ScenarioInterface).
  We could add a new interface on top, though - no BC break there.
• Still a bit uncomfortable requiring a migration from getTitle() to getScenarioTitle() getName().
• Could it make sense to make \Behat\Gherkin\Node\ExampleNode::replaceTextTokens public? It feels less important to do so with this PR, plus it's a BC break (since ExampleNode is not final)

[acoulton]

Sorry for the slow review, been a busy week.

Urgh, I hadn't appreciated how complex & nested the interfaces for these classes are. Considering that, I'd be nervous about adding yet another.

As I see it, the problem we're fighting is:

• Arguably, getTitle() has always returned the wrong thing for an ExampleNode - the idea implied by the interface is it looks and behaves like a Scenario, in which case the title should be the user-supplied title of the Scenario Outline, not the example values in text form.
• However, we can't change the result of getTitle() because there are existing valid uses - in Behat and probably elsewhere - that need the text example and rely on getting it from that method.
• And these are implementations of interfaces, not simple DTOs, so as you say, there's no easy way to provide a BC path to deprecate ambiguous method and add a new one.

So I think my suggestion for now is:

• Add the index to ExampleNode as planned.
• Add getName() to ExampleNode (instead of calling it getScenarioTitle()) - this would align with the naming in cucumber's AST.
• Add the getExampleText() as planned.
• Deprecate getTitle() only on ExampleNode but only with a docblock tag pointing to getName() or getExampleText(), not a runtime deprecation.
• Open an issue for a future major version to consider changing the interfaces so that anything "scenario-like" has a getName() instead of a getTitle(). I would probably do this alongside bumping Behat's major, so that we can be sure any extensions etc only get the Gherkin objects they were expecting even if they don't specify a behat/gherkin constraint of their own.

This means that:

• The change is BC for all existing users
• Only users who are writing formatters or similar and explicitly considering ExampleNode will see the deprecation (in their IDE or e.g. phpstan etc). But these are really the only cases at the moment where it's necessary to encourage people to be explicit about which one they want.
• If / when we release a new major renaming getTitle() to getName() then code that has already been updated for ExampleNode will be fine, anything that is only using the scenario interfaces will by default get the new (/expected) value instead of the table text, and we can highlight the difference in the upgrade notes.

This feels a bit messy, and I'm not sure it's right, but I think it probably is the least-worst compromise solution to adding this feature in a BC way.

Thoughts welcome!

[uuf6429]

@acoulton your points are most fine to me except this:

• Deprecate getTitle() only on ExampleNode but only with a docblock tag pointing to getName() or getExampleText(), not a runtime deprecation.
• Open an issue for a future major version to consider changing the interfaces so that anything "scenario-like" has a getName() instead of a getTitle().

Why wait? If we do that later, we won't have a chance to deprecate it - it will suddenly be renamed from getTitle to getName (unless we have to wait for 2 major versions). I would define getName in both scenario and example nodes.

Additionally, I would consider creating a new NamedNodeInterface or similar that defines getName and apply it to the current two classes - since both classes would have a getName, even if someone extends those classes, nothing would break. Later (in the major version bumpp) we could merge NamedNodeInterface into some existing interface or whatever. I don't think having an interface is too important though, end users could always use union types with PHPDoc.

The point is that it would nice (for users) to code against $scenarioOrExample->getName() instead of a lot of conditions - after the major version bump that part would remain unchanged.

---

Another interesting thing to note: I was considering deprecating getTitle at the interface level (it is defined in KeywordNodeInterface, but now I realised this is used by a lot more nodes. I'm wondering if we should implement getName for all of them already. 🤔
PS: I also noticed that there's an OutlineNode, (of course), what should we do about getTitle there?
PSS: I get that we might want to have a PR with the least amount of changes necessary, but the situation is already pretty complex and I'd like to avoid questions and potential reverts coming up in the next major version. I think we should be mindful about that too (e.g. that this change is well documented).

[acoulton]

@uuf6429 my reluctance to introduce new interfaces/deprecate things at interface level at this stage is exactly because I think it needs more thought about what that structure should be.

ISTR there are nodes currently defining getTitle where this is probably correct.

I think part of the problem is methods have been defined at the wrong level of the hierarchy because they appear to be similar. For example KeywordNode - most lines in the gherkin file have a keyword and some following text, but it doesn't follow that the meaning of that text is the same for all types of node - as we're finding now with ExampleNode where the text is actually not the title at all.

You can see from the existing Behat formatter implementations that actually it very rarely makes sense to call getTitle() on a KeywordNode without knowing exactly what type of node it is. That probably means the method is in the wrong place, or called the wrong thing.

But to deprecate it on the interface now, we need to find a well-designed and BC way to replace it across all the nodes that implement it : even though some nodes might actually be fine with getTitle, just through a different (or no) interface.

Really for this usecase what we want to capture is that Example is just a special kind of Scenario (possibly the only one). And they both have a name which is some text with a specific meaning & purpose.

So if we had to introduce an interface now I would call it NamedScenarioInterface and apply it only to ExampleNode and ScenarioNode.

I wouldn't use NamedNodeInterface because as with the existing interfaces, that implies nodes are similar because they happen to have a getName() method when we can see that isn't really true.

[acoulton]

Oh and on this specific:

Why wait? If we do that later, we won't have a chance to deprecate it - it will suddenly be renamed from getTitle to getName (unless we have to wait for 2 major versions).

I don't see us going immediately from this to a new major. Indeed with current resources a new major could still be a way off, especially if we're going to look at switching parser/moving to Pickles etc.

And even when we release a major there will be a period where we are maintaining both version series.

So there will still be an opportunity to deprecate methods between now and then - I just think we should minimise that until we can give users a clear overall explanation of what will be changing.

Deprecating the method on ExampleNode where it is currently ambiguous feels fine to do right now as there is an identified problem with that specific object and we have an easy solution.

[uuf6429]

@acoulton I agree with all points.

So if we had to introduce an interface now I would call it NamedScenarioInterface and apply it only to ExampleNode and ScenarioNode.

That makes sense to me, especially since most probably we'd want consistency over those two classes. What do you think about OutlineNode though? Are we going for Scenario or ScenarioLike?

Anyway, I'll implement this interface and open this PR for review, I think we're in a good state now for that.

[acoulton]

I think Outline is a higher level grouping of scenarios, like a feature or a suite (or a Rule if/when we support them).

It's not the same as a Scenario/Example which represents a specific set of executable steps.

So I would leave Outline alone, for the moment at least.

[uuf6429]

@acoulton side-note: there is a \Behat\Gherkin\Filter\NameFilter class which filters by title (despite the class name). 😅

[uuf6429]

@acoulton while adjusting tests, I found out that outline represents the "scenario outline" node and that, for example, iterating over scenarios in a feature file will actually yield instance of that object. Apparently it is up to caller to iterate over the "example [child] nodes" of that object while ignoring the rest of the object.

In short, a naive foreach ($scenarios as $scenario) echo $scenario->getName() would not work (although I'm not entirely sure it would make sense to support it). Because of that, I think I would implement getName()+NamedScenarioInterface on the outline class too (if we want to make it convenient). Worst case is that flattening the scenarios in the future would make that class (and its getName) disappear.