wip: Decoupling argument injection from store business #31

gorghoa · 2017-07-26T16:10:19Z

gorghoa

Some personal notes about my PR ;)

gorghoa · 2017-07-26T21:41:54Z

argumentsExt/Annotation/StepInjectorArgument.php

+     * @return string
+     */
+    public function getArgument();
+}


This interface may still be too related with the state sharing business 🤔

Why on earth should we force the annotation to deal with the getName? getArgument may be enough, but does it? Maybe we could get rid of it too…

gorghoa · 2017-07-26T21:42:34Z

argumentsExt/Argument/ArgumentOrganiser.php

     */
-    private $store;
+    private $hookers;


Sorry for the bad wordplay, couldn’t resist, will change that.

You are a man of public things !

gorghoa · 2017-07-26T21:43:07Z

argumentsExt/Argument/ArgumentOrganiser.php

@@ -59,16 +60,21 @@ public function organiseArguments(ReflectionFunctionAbstract $function, array $m
            return $this->baseOrganiser->organiseArguments($function, $match);
        }

-        /** @var ScenarioStateArgument[] $annotations */
+        /** @var StepArgumentInjectorArgument[] $annotations */


At this stage, it could be any type of annotation

gorghoa · 2017-07-26T21:44:23Z

argumentsExt/Argument/ArgumentOrganiser.php

-                $match[$annotation->getArgument()] = $store->getStateFragment($annotation->getName());
-                $match[strval(++$i)] = $store->getStateFragment($annotation->getName());
+                foreach ($this->hookers as $hooker) {
+                    if ($hooker->hasStepArgumentFor($annotation->getName())) {


this block should be handled by business service, not at framework level

gorghoa · 2017-07-26T21:46:40Z

argumentsExt/Argument/StepArgumentHolder.php

+ */
+interface StepArgumentHolder
+{
+    public function hasStepArgumentFor($key);


@@should be rename with something more meaningful like hasStepArgumentHandlerFor($key) or doesHandleStepArgument($key)

gorghoa · 2017-07-26T21:47:12Z

argumentsExt/Argument/StepArgumentHolder.php

+{
+    public function hasStepArgumentFor($key);
+
+    public function getStepArgumentFor($key);


should be rename by getStepArgumentValueFor($key), it would be more self explaining

gorghoa · 2017-07-26T21:48:20Z

argumentsExt/Resolver/ArgumentsResolver.php

            ) {
-                $arguments[$annotation->getArgument()] = $store->getStateFragment($annotation->getName());
+                foreach ($this->hookers as $hooker) {
+                    if ($hooker->hasStepArgumentFor($annotation->getName())) {


this if block is business logic and should be moved elsewhere in businessland.

gorghoa · 2017-07-26T21:51:26Z

argumentsExt/ServiceContainer/StepArgumentInjectorExtension.php

+            ->addMethodCall('addGlobalIgnoredName', ['AfterFeature'])
+            ->addMethodCall('addGlobalIgnoredName', ['AfterSuite']);
+
+        $taggedServices = array_map(function ($serviceId) {


this $taggedServices is boilerplate copy/pasting and should be renamed. Maybe something like $stepArgumentInjectorPluggedServices?

It's a local variable, you can name it as you want (but please not a too long variable name 😉 )

Simperfit · 2017-07-27T06:30:54Z

argumentsExt/Resolver/ArgumentsResolver.php

     */
-    private $reader;
+    private $hookers;


don't think that's the best word in the world

And that’s too bad, cause i find it a perfectly suitable naming for our needs :'(

(https://en.wiktionary.org/wiki/hooker)

The 1st definition is quite what I’m looking for…: “One who, or that which, hooks.”

But be at ease, i see the politically incorrect and discomfort that this naming carry.

Simperfit · 2017-07-27T06:31:22Z

src/.php_cs.cache

@@ -0,0 +1 @@
+{"php":"7.1.7-1+0~20170711133214.5+stretch~1.gbp5284f4","version":"2.4.0:v2.4.0#63661f3add3609e90e4ab8115113e189ae547bb4","rules":{"binary_operator_spaces":{"align_double_arrow":false,"align_equals":false},"blank_line_after_opening_tag":true,"blank_line_before_statement":{"statements":["return"]},"braces":{"allow_single_line_closure":true},"cast_spaces":true,"class_definition":{"singleLine":true},"concat_space":{"spacing":"none"},"declare_equal_normalize":true,"function_typehint_space":true,"hash_to_slash_comment":true,"include":true,"lowercase_cast":true,"magic_constant_casing":true,"method_argument_space":true,"method_separation":true,"native_function_casing":true,"new_with_braces":true,"no_blank_lines_after_class_opening":true,"no_blank_lines_after_phpdoc":true,"no_empty_comment":true,"no_empty_phpdoc":true,"no_empty_statement":true,"no_extra_consecutive_blank_lines":{"tokens":["curly_brace_block","extra","parenthesis_brace_block","square_brace_block","throw","use"]},"no_leading_import_slash":true,"no_leading_namespace_whitespace":true,"no_mixed_echo_print":{"use":"echo"},"no_multiline_whitespace_around_double_arrow":true,"no_short_bool_cast":true,"no_singleline_whitespace_before_semicolons":true,"no_spaces_around_offset":true,"no_trailing_comma_in_list_call":true,"no_trailing_comma_in_singleline_array":true,"no_unneeded_control_parentheses":true,"no_unused_imports":true,"no_whitespace_before_comma_in_array":true,"no_whitespace_in_blank_line":true,"normalize_index_brace":true,"object_operator_without_whitespace":true,"php_unit_fqcn_annotation":true,"phpdoc_align":true,"phpdoc_annotation_without_dot":true,"phpdoc_indent":true,"phpdoc_inline_tag":true,"phpdoc_no_access":true,"phpdoc_no_alias_tag":true,"phpdoc_no_empty_return":true,"phpdoc_no_package":true,"phpdoc_no_useless_inheritdoc":true,"phpdoc_return_self_reference":true,"phpdoc_scalar":true,"phpdoc_separation":true,"phpdoc_single_line_var_spacing":true,"phpdoc_summary":true,"phpdoc_to_comment":true,"phpdoc_trim":true,"phpdoc_types":true,"phpdoc_var_without_name":true,"pre_increment":true,"protected_to_private":true,"return_type_declaration":true,"self_accessor":true,"short_scalar_cast":true,"single_blank_line_before_namespace":true,"single_class_element_per_statement":true,"single_quote":true,"space_after_semicolon":true,"standardize_not_equals":true,"ternary_operator_spaces":true,"trailing_comma_in_multiline_array":true,"trim_array_spaces":true,"unary_operator_spaces":true,"whitespace_after_comma_in_array":true,"blank_line_after_namespace":true,"elseif":true,"function_declaration":true,"indentation_type":true,"line_ending":true,"lowercase_constants":true,"lowercase_keywords":true,"no_break_comment":true,"no_closing_tag":true,"no_spaces_after_function_name":true,"no_spaces_inside_parenthesis":true,"no_trailing_whitespace":true,"no_trailing_whitespace_in_comment":true,"single_blank_line_at_eof":true,"single_import_per_statement":true,"single_line_after_imports":true,"switch_case_semicolon_to_colon":true,"switch_case_space":true,"visibility_required":true,"encoding":true,"full_opening_tag":true},"hashes":{"\/ScenarioStateInterface.php":2824836438,"\/Annotation\/ScenarioStateArgument.php":2979727378,"\/Context\/Initializer\/ScenarioStateInitializer.php":781987277,"\/Context\/ScenarioStateAwareTrait.php":2441401241,"\/Context\/ScenarioStateAwareContext.php":1438795547,"\/ScenarioState.php":3549039778,"\/ServiceContainer\/ScenarioStateExtension.php":1698856486,"\/Exception\/MissingStateException.php":1656618837}}


I don't think this should be committed

vincentchalamon · 2017-07-28T07:10:24Z

argumentsExt/Argument/ArgumentOrganiser.php

-            if ($annotation instanceof ScenarioStateArgument &&
-                in_array($annotation->getArgument(), $paramsKeys) &&
-                $store->hasStateFragment($annotation->getName())
+            if ($annotation instanceof StepInjectorArgument &&


if (!$annotation instanceof StepInjectorArgument || !in_array($argument = $annotation->getArgument(), $paramsKeys) ) { continue; }

vincentchalamon · 2017-07-28T07:12:11Z

argumentsExt/Resolver/ArgumentsResolver.php

-        // No `@ScenarioStateArgument` annotation found
-        if (null === $this->reader->getMethodAnnotation($function, ScenarioStateArgument::class)) {
+        // No `@StepInjectorArgument` annotation found
+        if (null === $this->reader->getMethodAnnotation($function, StepInjectorArgument::class)) {


StepInjectorArgument is not an annotation

vincentchalamon · 2017-07-28T07:12:47Z

argumentsExt/Resolver/ArgumentsResolver.php

-            if ($annotation instanceof ScenarioStateArgument &&
-                in_array($annotation->getArgument(), $paramsKeys) &&
-                $store->hasStateFragment($annotation->getName())
+            if ($annotation instanceof StepInjectorArgument &&


if (!$annotation instanceof StepInjectorArgument || !in_array($argument = $annotation->getArgument(), $paramsKeys) ) { continue; }

Duplicated code from ArgumentOrganiser

vincentchalamon · 2017-07-28T07:14:00Z

argumentsExt/ServiceContainer/StepArgumentInjectorExtension.php

+     */
+    public function getConfigKey()
+    {
+        return 'stepargumentinjector';


step_argument_injector

vincentchalamon · 2017-07-28T07:14:50Z

argumentsExt/ServiceContainer/StepArgumentInjectorExtension.php

+        // Declare Doctrine annotation reader as service
+        $container->register(self::STEP_ARGUMENT_INJECTOR_DOCTRINE_READER_ID, AnnotationReader::class)
+            // Ignore Behat annotations in reader
+            ->addMethodCall('addGlobalIgnoredName', ['Given'])


According to #32 (comment), you should also ignore lowercase given (same for all Behat annotations).
BTW: does Behat works if I declare my annotation as @gIvEn?

vincentchalamon · 2017-07-28T07:16:19Z

argumentsExt/ServiceContainer/StepArgumentInjectorExtension.php

+            ->addMethodCall('addGlobalIgnoredName', ['AfterFeature'])
+            ->addMethodCall('addGlobalIgnoredName', ['AfterSuite']);
+
+        $taggedServices = array_map(function ($serviceId) {


It's a local variable, you can name it as you want (but please not a too long variable name 😉 )

vincentchalamon · 2017-07-28T07:16:54Z

argumentsExt/ServiceContainer/StepArgumentInjectorExtension.php

+
+        // Argument organiser
+        $container->register(self::STEP_ARGUMENT_INJECTOR_ARGUMENT_ORGANISER_ID, ArgumentOrganiser::class)
+            ->setDecoratedService(ArgumentExtension::PREG_MATCH_ARGUMENT_ORGANISER_ID)


We should find a better way to extend Behat without decorating its kernel services.
Or Behat allows us to extend its kernel more easily (PR to open), or we could create another extension which implements this extend for any extension. WDYT @gorghoa?

vincentchalamon · 2017-07-28T07:16:58Z

argumentsExt/ServiceContainer/StepArgumentInjectorExtension.php

+
+        // Override calls process
+        $container->register(self::STEP_ARGUMENT_INJECTOR_CALL_HANDLER_ID, RuntimeCallHandler::class)
+            ->setDecoratedService(CallExtension::CALL_HANDLER_TAG.'.runtime')


gorghoa added the Work In Progress label Jul 26, 2017

gorghoa force-pushed the research/30-decoupling branch 2 times, most recently from f5c2672 to 6069681 Compare July 26, 2017 16:14

wip: behat tests are dirty passing, keep going

a7bbc31

gorghoa force-pushed the research/30-decoupling branch from 6069681 to a7bbc31 Compare July 26, 2017 21:33

gorghoa commented Jul 26, 2017

View reviewed changes

Simperfit reviewed Jul 27, 2017

View reviewed changes

Delegate annotation handling to hooked services

ae55309

gorghoa force-pushed the research/30-decoupling branch from b4de9cd to ae55309 Compare July 27, 2017 10:19

Decouple phpunit tests

b83b775

gorghoa force-pushed the research/30-decoupling branch from 9b21c84 to b83b775 Compare July 27, 2017 19:33

removing .php_cs.cache

00ac897

vincentchalamon suggested changes Jul 28, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wip: Decoupling argument injection from store business #31

wip: Decoupling argument injection from store business #31

gorghoa commented Jul 26, 2017

gorghoa left a comment

gorghoa Jul 26, 2017 •

edited

Loading

gorghoa Jul 26, 2017

Simperfit Jul 27, 2017 •

edited

Loading

gorghoa Jul 26, 2017

gorghoa Jul 26, 2017

gorghoa Jul 26, 2017 •

edited by Simperfit

Loading

gorghoa Jul 26, 2017

gorghoa Jul 26, 2017

gorghoa Jul 26, 2017

vincentchalamon Jul 28, 2017

Simperfit Jul 27, 2017

gorghoa Jul 27, 2017

gorghoa Jul 27, 2017

Simperfit Jul 27, 2017

gorghoa Jul 27, 2017 •

edited

Loading

vincentchalamon Jul 28, 2017

vincentchalamon Jul 28, 2017

vincentchalamon Jul 28, 2017

vincentchalamon Jul 28, 2017

vincentchalamon Jul 28, 2017

vincentchalamon Jul 28, 2017

vincentchalamon Jul 28, 2017

vincentchalamon Jul 28, 2017

vincentchalamon Jul 28, 2017

		@@ -0,0 +1 @@
		{"php":"7.1.7-1+0~20170711133214.5+stretch~1.gbp5284f4","version":"2.4.0:v2.4.0#63661f3add3609e90e4ab8115113e189ae547bb4","rules":{"binary_operator_spaces":{"align_double_arrow":false,"align_equals":false},"blank_line_after_opening_tag":true,"blank_line_before_statement":{"statements":["return"]},"braces":{"allow_single_line_closure":true},"cast_spaces":true,"class_definition":{"singleLine":true},"concat_space":{"spacing":"none"},"declare_equal_normalize":true,"function_typehint_space":true,"hash_to_slash_comment":true,"include":true,"lowercase_cast":true,"magic_constant_casing":true,"method_argument_space":true,"method_separation":true,"native_function_casing":true,"new_with_braces":true,"no_blank_lines_after_class_opening":true,"no_blank_lines_after_phpdoc":true,"no_empty_comment":true,"no_empty_phpdoc":true,"no_empty_statement":true,"no_extra_consecutive_blank_lines":{"tokens":["curly_brace_block","extra","parenthesis_brace_block","square_brace_block","throw","use"]},"no_leading_import_slash":true,"no_leading_namespace_whitespace":true,"no_mixed_echo_print":{"use":"echo"},"no_multiline_whitespace_around_double_arrow":true,"no_short_bool_cast":true,"no_singleline_whitespace_before_semicolons":true,"no_spaces_around_offset":true,"no_trailing_comma_in_list_call":true,"no_trailing_comma_in_singleline_array":true,"no_unneeded_control_parentheses":true,"no_unused_imports":true,"no_whitespace_before_comma_in_array":true,"no_whitespace_in_blank_line":true,"normalize_index_brace":true,"object_operator_without_whitespace":true,"php_unit_fqcn_annotation":true,"phpdoc_align":true,"phpdoc_annotation_without_dot":true,"phpdoc_indent":true,"phpdoc_inline_tag":true,"phpdoc_no_access":true,"phpdoc_no_alias_tag":true,"phpdoc_no_empty_return":true,"phpdoc_no_package":true,"phpdoc_no_useless_inheritdoc":true,"phpdoc_return_self_reference":true,"phpdoc_scalar":true,"phpdoc_separation":true,"phpdoc_single_line_var_spacing":true,"phpdoc_summary":true,"phpdoc_to_comment":true,"phpdoc_trim":true,"phpdoc_types":true,"phpdoc_var_without_name":true,"pre_increment":true,"protected_to_private":true,"return_type_declaration":true,"self_accessor":true,"short_scalar_cast":true,"single_blank_line_before_namespace":true,"single_class_element_per_statement":true,"single_quote":true,"space_after_semicolon":true,"standardize_not_equals":true,"ternary_operator_spaces":true,"trailing_comma_in_multiline_array":true,"trim_array_spaces":true,"unary_operator_spaces":true,"whitespace_after_comma_in_array":true,"blank_line_after_namespace":true,"elseif":true,"function_declaration":true,"indentation_type":true,"line_ending":true,"lowercase_constants":true,"lowercase_keywords":true,"no_break_comment":true,"no_closing_tag":true,"no_spaces_after_function_name":true,"no_spaces_inside_parenthesis":true,"no_trailing_whitespace":true,"no_trailing_whitespace_in_comment":true,"single_blank_line_at_eof":true,"single_import_per_statement":true,"single_line_after_imports":true,"switch_case_semicolon_to_colon":true,"switch_case_space":true,"visibility_required":true,"encoding":true,"full_opening_tag":true},"hashes":{"\/ScenarioStateInterface.php":2824836438,"\/Annotation\/ScenarioStateArgument.php":2979727378,"\/Context\/Initializer\/ScenarioStateInitializer.php":781987277,"\/Context\/ScenarioStateAwareTrait.php":2441401241,"\/Context\/ScenarioStateAwareContext.php":1438795547,"\/ScenarioState.php":3549039778,"\/ServiceContainer\/ScenarioStateExtension.php":1698856486,"\/Exception\/MissingStateException.php":1656618837}}

wip: Decoupling argument injection from store business #31

Are you sure you want to change the base?

wip: Decoupling argument injection from store business #31

Conversation

gorghoa commented Jul 26, 2017

gorghoa left a comment

Choose a reason for hiding this comment

gorghoa Jul 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Simperfit Jul 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gorghoa Jul 26, 2017 • edited by Simperfit Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gorghoa Jul 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gorghoa Jul 26, 2017 •

edited

Loading

Simperfit Jul 27, 2017 •

edited

Loading

gorghoa Jul 26, 2017 •

edited by Simperfit

Loading

gorghoa Jul 27, 2017 •

edited

Loading