feature: Add support for custom method placement in ordered_class_elements

[bakerkretzmar]

As discussed in #6244, this PR adds the ability to place specific individual methods in a custom order within a class.

The original PR was to add a custom invoke option similar to construct, to place the __invoke() method at a custom location instead of grouping it with other magic methods. After some discussion with @SpacePossum @gharlan and @driftingly it sounded like a more generic solution would be preferred, to allow placing any specific method at a custom location, not just __invoke().

The suggestion from that discussion was to allow ['method' => 'methodName'] in the list of options to specify custom orders. I chose to use method:methodName instead because the code changes are a lot simpler and smaller if all the options are still plain strings.

method: options override any other ordering, so for example method:__invoke takes precedence over magic.

In the future it would probably be quite straightforward to support specific properties/constants/traits too with similar syntax (e.g. property:propertyName), although I'm not sure if that's useful.

Replaces #6244
Closes #6252

[driftingly]

Thank you @bakerkretzmar 🙌

[bakerkretzmar]

@keradus @SpacePossum @gharlan anything else you need from me here?

[driftingly]

Thanks, @bakerkretzmar for updating this!

[Wirone]

@bakerkretzmar I wanted to rebase it in order to process it further on latest master, but it seems like your changes in docs were made by hand, not with dev-tools/doc.php (how checks have passed 🤔?). There's no way to achieve "subset of" allowed values with callable, which is unfortunate. It would be great to find a way to document method:* in user-friendly manner. I really would like to get this merged, I really like the idea 🙂.

[bakerkretzmar]

@Wirone thanks! I did update the docs by hand, I had no idea dev-tools/doc.php existed 😄 I'm not sure how to document method:* automatically... I based the closure in this PR on some others I found in the codebase, like these:

PHP-CS-Fixer/src/Fixer/ClassNotation/ClassAttributesSeparationFixer.php

Lines 209 to 238 in c1470f1

	->setAllowedValues([static function (array $option): bool {
	foreach ($option as $type => $spacing) {
	$supportedTypes = ['const', 'method', 'property', 'trait_import', 'case'];
	if (!\in_array($type, $supportedTypes, true)) {
	throw new InvalidOptionsException(
	sprintf(
	'Unexpected element type, expected any of "%s", got "%s".',
	implode('", "', $supportedTypes),
	\gettype($type).'#'.$type
	)
	);
	}
	$supportedSpacings = [self::SPACING_NONE, self::SPACING_ONE, self::SPACING_ONLY_IF_META];
	if (!\in_array($spacing, $supportedSpacings, true)) {
	throw new InvalidOptionsException(
	sprintf(
	'Unexpected spacing for element type "%s", expected any of "%s", got "%s".',
	$spacing,
	implode('", "', $supportedSpacings),
	\is_object($spacing) ? \get_class($spacing) : (null === $spacing ? 'null' : \gettype($spacing).'#'.$spacing)
	)
	);
	}
	}
	return true;
	}])

PHP-CS-Fixer/src/Fixer/PhpUnit/PhpUnitTestCaseStaticMethodCallsFixer.php

Lines 373 to 398 in c1470f1

	->setAllowedValues([static function (array $option) use ($thisFixer): bool {
	foreach ($option as $method => $value) {
	if (!isset($thisFixer->staticMethods[$method])) {
	throw new InvalidOptionsException(
	sprintf(
	'Unexpected "methods" key, expected any of "%s", got "%s".',
	implode('", "', array_keys($thisFixer->staticMethods)),
	\gettype($method).'#'.$method
	)
	);
	}
	if (!isset($thisFixer->allowedValues[$value])) {
	throw new InvalidOptionsException(
	sprintf(
	'Unexpected value for method "%s", expected any of "%s", got "%s".',
	$method,
	implode('", "', array_keys($thisFixer->allowedValues)),
	\is_object($value) ? \get_class($value) : (null === $value ? 'null' : \gettype($value).'#'.$value)
	)
	);
	}
	}
	return true;
	}])

PHP-CS-Fixer/src/Fixer/Import/OrderedImportsFixer.php

Lines 260 to 282 in c1470f1

	->setAllowedValues([static function (?array $value) use ($supportedSortTypes): bool {
	if (null !== $value) {
	$missing = array_diff($supportedSortTypes, $value);
	if (\count($missing) > 0) {
	throw new InvalidOptionsException(sprintf(
	'Missing sort %s "%s".',
	1 === \count($missing) ? 'type' : 'types',
	implode('", "', $missing)
	));
	}
	$unknown = array_diff($value, $supportedSortTypes);
	if (\count($unknown) > 0) {
	throw new InvalidOptionsException(sprintf(
	'Unknown sort %s "%s".',
	1 === \count($unknown) ? 'type' : 'types',
	implode('", "', $unknown)
	));
	}
	}
	return true;
	}])

How are those ones documented?

[Wirone]

I had no idea dev-tools/doc.php existed 😄

I don't know if it existed back then, so no worry. That's why I try to introduce some common scripts and improve contributing guide, because I see that not everything is obvious (for me too) 🙂.

In terms of allowed values probably we need to include info about method:* in the fixer's description ("List of strings defining order of elements.") or add it to $specialTypes so it's automatically generated along with other values? I honestly don't know, I still learn 😅. @kubawerlos can you suggest something?

[kubawerlos]

Before we think how, let's discuss if we should do it at all, see #3864 (comment).

[kubawerlos]

Closing/reopening to trigger CI.

[Wirone]

@kubawerlos I don't see negative sides of this approach (besides slightly higher complexity to maintain). There's no any "injecting code", only fixed implementation that allows better flexibility of how users want to order methods in a class. It's option like the others, but supports dynamic values - however those values have to be in subset defined by class itself. I believe there are valid scenarios for that 🙂.

[kubawerlos]

@Wirone oh, I see now, I understood the docs part:

a single method name (e.g. method:__invoke) to set the relative order of that particular method

as there would be possible to use that method to sort class elements, assuming the doc was generated from code, which apparently is not the case here.

There is no way to achieve that now - it's either a fixed list of strings or any string.

One way to go would be to deprecate the current option and create a new one, where all current options would have @ at the beginning or maybe something smarter, not to confuse with PHPDoc tags, let's say # for a moment, and other entries would be interpreted as methods, so config can look like:

[
    '#method_protected_static',
    'MyClass::myMethod',
    '#method_protected',
    '#magic'
]

SRP for the win - let's split it anyway and add magic (and phpunit, but I have no clue what it suppose to do) with proper tests for each new type.

[bakerkretzmar]

@kubawerlos that's an interesting idea! I'd like to keep the scope of this PR very focused, so I'm not going to update it to deprecate the current options—my proposal here is only to add a new method: prefix to adjust the order of specific methods.

@Wirone can this particular fixer just be documented by hand?

Let me know if there's anything I need to do here!

[Wirone]

can this particular fixer just be documented by hand?

No, because it will be overridden by script when it' used. We need to use description for that, so script would take it always the same way. And it won't pass auto-review tests, which use the same mechanism as dev-tools/doc.php for generating docs and will assert if everything is in sync.

[bakerkretzmar]

@Wirone thanks! I moved things into the description, the documentation script should work now.

[Wirone]

👍 from me, good job @bakerkretzmar. I won't merge it, though, let's wait for another review.

[Wirone]

Thank you @bakerkretzmar and welcome to the Fixer's contributors group 😁🍻.

[bakerkretzmar]

Thanks!! Really appreciate all the feedback on this and glad we're going to be able to use it soon 🔨 🔧