Skip to content

Conversation

carlos-granados
Copy link
Contributor

Currently we have several places in our code where we handle paths to relativize them. This PR replaces them with a single service that can print paths and which is configurable. And we add a new option which can be used to tell Behat to print all paths as absolute instead of relative paths. This option is available on the command line and also in the Php and Yaml configs

@@ -83,6 +84,7 @@ protected function getDefaultExtensions()
new AutoloaderExtension(['' => '%paths.base%/features/bootstrap']),
new SuiteExtension($processor),
new OutputExtension('pretty', $this->getDefaultFormatterFactories($processor), $processor),
new PathOptionsExtension(),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a new extension that adds the new path options

*/
public function __construct(
ResultToStringConverter $resultConverter,
ExceptionPresenter $exceptionPresenter,
TranslatorInterface $translator,
$basePath,
string $basePath,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This parameter is not used any more because the configurable path printer service takes care of handling path printing

@@ -281,35 +284,19 @@ private function printStepStat(
$printer->writeln();
}

/**
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And now this function is not needed

@@ -220,6 +220,7 @@ protected function loadCorePrinters(ContainerBuilder $container)
new Reference('output.node.printer.pretty.width_calculator'),
'%paths.base%',
]);
$definition->addMethodCall(('setConfigurablePathPrinter'), [new Reference('configurable.path.printer')]);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We inject the new service into the classes that need it

@@ -102,6 +106,13 @@ public function withPrintUnusedDefinitions(bool $printUnusedDefinitions = true):
return $this;
}

public function withPathOptions(bool $printAbsolutePaths): self
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New config option to set this option using PHP

@@ -215,6 +227,22 @@ private function addUnusedDefinitionsToExpr(Expr &$expr): void
}
}

private function addPathOptionsToExpr(Expr &$expr): void
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And new yaml to php converter for this option


namespace Behat\Testwork\PathOptions\Printer;

final class ConfigurablePathPrinter
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the new service that prints paths, adding an option to decide if they should be printed as relative (default) or absolute

Copy link
Contributor

@acoulton acoulton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be great @carlos-granados, thanks. Just a couple of thoughts.

$this->basePath = $basePath;
}

public function setConfigurablePathPrinter(ConfigurablePathPrinter $configurablePathPrinter)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little nervous that there might still be a BC impact with some of these classes if they're used by extensions / formatters and:

  • They aren't taking them through our service container, so this method doesn't get called and class fails attempting to call the ->configurablePathPrinter
  • They are using our service container definitions, but decorating our class, so this method won't be defined on their decorator and the service container will throw on attempting to create the service.

I wonder if we can make this safer.

Perhaps if we instead add this as an optional constructor parameter with a fallback instead of a setter - e.g. like this:

public function __construct(
        private ResultToStringConverter $resultConverter,
        private ExceptionPresenter $exceptionPresenter,
        private TranslatorInterface $translator,
        string $basePath,
        ?ConfigurablePathPrinter $configurablePathPrinter = null
) {
        $this->configurablePathPrinter = $configurablePathPrinter ?? new ConfigurablePathPrinter($basePath, printAbsolutePaths: false);
    }

Then I think:

  • If people are doing new ListPrinter(...) it will continue to work exactly as now, it just won't respect the new config option
  • If people have decorated our class without changing the constructor args, they'll get an extra argument that is not in their constructor signature and ignore it
  • If people have decorated our class and changed the constructor args, they will get the same class / args as before

What do you think?

@acoulton
Copy link
Contributor

I think this will also help with #1601

@carlos-granados carlos-granados force-pushed the print-absolute-paths branch 3 times, most recently from 18c691c to 1f4998f Compare April 16, 2025 08:28
@carlos-granados
Copy link
Contributor Author

@acoulton regarding your comment about the constructors, agree that your suggestion might be a better idea, updated. I also added a constant for the id of the configurable path printer service

Copy link
Contributor

@acoulton acoulton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks @carlos-granados - just one tiny thing I spotted in the diff.

@acoulton acoulton merged commit e2c55f9 into Behat:master Apr 16, 2025
18 checks passed
@carlos-granados carlos-granados deleted the print-absolute-paths branch April 21, 2025 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants