use entity type hinting in routes

That was my original plan, but autoloading entities with two consecutive URL parameters was hopelessly broken when I tried it. We would have to change the route to something like /flag/flag/{flag}/flaggable/{entity} to separate the two parameters.

Now if we can get two consecutive parameters to work, sure! If we can simplify the URLs overall, that'd also be great. My prime concern then was to replicate the existing paths in the 7.x module.

Something like that. Although at the time I think it was just unresponsive.

That error sounds like you just forget to rename flag_id to flag somewhere where that route was used. The way to debug this is search for the route name, which I guess will point you to the link type plugins, which in turn points to http://cgit.drupalcode.org/flag/tree/src/ActionLinkTypeBase.php?h=8.x-4.....

Debugging in those method only helps to figure out the route that is the problem, because the rendering/building happens indirectly, what you need to find is where it is defined initially.

Yes, there's a lot of indirection in place, but to be fair, a considerable amount of that is in flag module itself, in those link plugins, that separates the route name into a separate method, so that route name and parameters aren't in the same place. Which saves a few lines of code and means you have to update a single place instead of 4, but that single place in turn is harder to find. There's always a tradeoff :)

ID is correct, sounds like you then actually get further and run into the next problem that you're just not seeing otherwise because of the exception.

Now you need to update FlaggingAccessController. Right now, you have a $flag_id argument there (which the controller is not passing in, then a recoverable fatal error happens but it continues), you need to change that to FlagInterface $flag and then you don't have to load it anymore.

+++ b/src/Controller/AJAXLinkController.php
@@ -33,7 +33,9 @@ class AJAXLinkController extends ControllerBase {
-  public function flag($flag_id, $entity_id) {
+  public function flag($flag, $entity_id) {
+    $flag_id = $flag->id();

Entity are only upcasted if you added the type hint, so you need to add that everywhere.

Nifty!

Just providing information - obtained from a stack trace obtained by running Drupal\flag\Tests\FlagFieldEntryTest

The first fail is the result of a page request to 'node/1' with the expectation of displaying a newly created node.

I will try and track this down some more in a few days when I get more time.. I can't yet explain why the node write/read cycle fails
but tonight I am going to comment on another aspect of it.

The user will occasionally supply an invalid id - which is fine. but our code has a brittle way of dealing with it..

From the stack trace the first argument injected into FieldEntryFormController::getForm() is a boolen (false)

whereas the type hinting is demanding a ( FlaggingInterface $flagging )

The mismatch occurs when FieldEntryFormController::getFlagging() fails to find its target.
and returns the equivalent of reset( "empty array" ) which is understandably false.
As the code as it stands the return type of FieldEntryFormController::getFlagging() is incorrectly annotated and should read.

@return FlaggingInterface|bool
  The flagging or FALSE

Anyway in the spirit of post often -- I hope this was worth reading.

Fails several times for me locally.

FlagConfirmFormTest.php line 151:

Recoverable fatal error: Argument 1 passed to Drupal\flag\FlagService::getFlaggableById() must implement interface Drupal\flag\FlagInterface, null given, called in /home/tess/sites/drupal8/modules/flag/src/Form/FlaggingConfirmForm.php on line 43 and defined in Drupal\flag\FlagService->getFlaggableById() (line 218 of modules/flag/src/FlagService.php).

FlagFieldEntryTest.php line 203:

Symfony\Component\Routing\Exception\MissingMandatoryParametersException: Some mandatory parameters are missing ("flag") to generate a URL for route "/flag/confirm/unflag/{flag}/{entity_id}". in Symfony\Component\Routing\Generator\UrlGenerator->doGenerate() (line 155 of core/vendor/symfony/routing/Symfony/Component/Routing/Generator/UrlGenerator.php).

FlagFieldEntryTest.php line 219:

Symfony\Component\Routing\Exception\MissingMandatoryParametersException: Some mandatory parameters are missing ("flag") to generate a URL for route "/flag/confirm/unflag/{flag}/{entity_id}". in Symfony\Component\Routing\Generator\UrlGenerator->doGenerate() (line 155 of core/vendor/symfony/routing/Symfony/Component/Routing/Generator/UrlGenerator.php

Status Postponed

That means the branch tests aren't passing, so it doesn't run tests for patches. Was an infrastructure problem (git repository checkout failed), happens a lot lately.

From #15

Can you file a new issue for that?

done.

#2464995: FieldEntryFormController::getFlagging() change the 'unfound' return type from FALSE to NULL

1. +++ b/src/Controller/AJAXLinkController.php
   @@ -23,8 +23,8 @@ class AJAXLinkController extends ControllerBase {
   +   * @param FlagInterface $flag
   +   *   The flag entity.
   

   per coding standards, type class names should be fully namespaces in @param and @return.

2. +++ b/src/Controller/AJAXLinkController.php
   @@ -33,7 +33,9 @@ class AJAXLinkController extends ControllerBase {
   +    $flag_id = $flag->id();
   +
        $flagging = \Drupal::service('flag')->flag($flag_id, $entity_id);
   

   Why don't you just write $flag->id() when passing it to the method? This seems much more complicated to me like this.

3. +++ b/src/Controller/FieldEntryFormController.php
   @@ -28,7 +29,9 @@ class FieldEntryFormController extends ControllerBase {
   +  public function flag(FlagInterface $flag, $entity_id) {
   +    $flag_id = $flag->id();
   +
        $account = $this->currentUser();
        $flag = Flag::load($flag_id);
   

   Uhm ;)

4. +++ b/src/Controller/FieldEntryFormController.php
   @@ -82,16 +89,16 @@ class FieldEntryFormController extends ControllerBase {
   -  public function flagTitle($flag_id, $entity_id) {
   -    $flag = \Drupal::service('flag')->getFlagById($flag_id);
   +  public function flagTitle(FlagInterface $flag, $entity_id) {
   +    //$flag = \Drupal::service('flag')->getFlagById($flag_id);
   

   And one more commented out line that can be dropped.

5. +++ b/src/Controller/FieldEntryFormController.php
   @@ -99,16 +106,16 @@ class FieldEntryFormController extends ControllerBase {
   +    //$flag = \Drupal::service('flag')->getFlagById($flag_id);
   

   Same here.

6. +++ b/src/Form/FlaggingConfirmForm.php
   @@ -35,10 +36,11 @@ class FlaggingConfirmForm extends ConfirmFormBase {
      public function buildForm(array $form, FormStateInterface $form_state,
   -                            $flag_id = NULL, $entity_id = NULL) {
   +                            FlagInterface $flag = NULL, $entity_id = NULL) {
    
   +    $this->flag = $flag;
   +    $flag_id = $flag->id();
        $flag_service = \Drupal::service('flag');
   

   $flag_id is never used here?

I wanted to see an interdiff .. might a well post it in public.

Overall - I like the improvement this patch offers...

Just ironing out a little kink - we were loading a flag object that has already been magically loaded.

Tests are now passing, but this probably needs human review too.

I am slowly doing this sorry if it comes out in drips and drabs.

3 observations :-

A) Implicit to these changes is a magic entity load before the constructor is called. - and mostly these changes are justified as the entity is needed anyways to complete the task.

B) We automatically pick some 'reject the request early with a 404' protection if the flag id is invalid. [ which is very good :) ]

C) A common pattern is that the controller extracts the flag id and feeds it into the FlagService::flag()/unflag() methods which load the entity again!

Code changes :-

1) (C) can be used to cut out unnecessary calls to load the flag entity.

2) FIeldEntryFormController::edit()/unflag(); (A) does not apply. So we could revert back to using flag_id and saved a redundant load. I personally value the extra protection from (B) - in short no change.

3) FIeldEntryFormController::getFlagging() has been converted. the entity is pre-loaded and $flag = \Drupal::service('flag')->getFlagById($flag_id); removed
so the only real advantage is (B)

4) Where appropriate I have prefixed $flag to become FlagInterface $flag in a few function arguments.

5) Where appropriate I have fixed up a few function annotations.

6) AJAXLinkController::flag() - I have removed another unnecessary call to flagging->getFlag() as the flag entity is unmodified by the function and the flag input to the function can be used directly in the output.

7) AJAXLinkController::unflag() - flag is unmodified by the unflag operation and so the flag input to the function can be used directly in the output.

8) ReloadLinkController - the input flag entity is used to extract the id which is fed into FlagService::flag() where the id is used to load the flag.. so there is duplication of work.

From the issue summary

But then I don't really follow the reason for the flag/unflag logic to live in FlagService rather than the flag class like it does on 7.

To me (C) and (1) amplify the call to move flag/unflag back into the Flag entity.

There are so many changes here - that this is becoming unreviewable.... so I will move the flag/unflag back in the the Flag entity in my next update early in the week.

PS I was about to file a duplicate of #2465095: classes in @param and @return must be fully-namespaced but caught myself :) So I am linking the issues to stop my brain from doing that again!

Parts of the last patch are definitely useful and I also tried to point out some of them in my review but it was probably not clear enough in some cases, like 3.

Quite a few controllers now get the flag, then get the ID to load the flag again. We should remove that here IMHO. I don't see why we would have to wait for follow-up's on that, it's something that is introduced here and should be cleaned up here.

I agree that the other part to optimize for not loading the entity is not worth it. in most cases, we will sooner or later need the flag anyway, and it's fairly well cached anyway.

So early next week I will start from 25 -

fix 3 is still valid - and can be put in with little fuss.

A smaller set of changes starting from #25.

Only uncontroversial ones... I hope

Reintroduced conversion of FieldEntryFormController:getFlagging which is #23.3
restricting myself to the smallest change possible in the function calling it FieldEntryFormController:edit() and FieldEntryFormController:unflag()

as part of this some annotations and function arguments have changed -- so that looks like #23.4 and #23.5 has also been picked up.

Next week I will resolve

Quite a few controllers now get the flag, then get the ID to load the flag again. We should remove that here IMHO. I don't see why we would have to wait for follow-up's on that, it's something that is introduced here and should be cleaned up here.

I hope this is acceptable -- I will back out anything you gauge inappropriate.

C) A common pattern is that the controller extracts the flag id and feeds it into the FlagService::flag()/unflag() methods which load the entity again!

There are flagByObject()/unflagByObject() methods on FlagService. We should be calling those when we have the obejcts in question rather than calling flag() or unflag(). All flag() and unflag() do is load the objects and call the zzzzByObject() method anyways.

If only PHP supported proper method overloading (http://php.net/manual/en/language.oop5.overloading.php) we wouldn't have this problem, we could just have the same method name with different arguments. Since I don't want to give up type hinting, a different named method is what was needed.

@socketwench - Thanks for filing those issues. Your wording is much more eloquent that what I had in mind :)

Leaving ANY issue relating to getFlagging and flag/unflag alone - well what remains if I have looked carefully enough is only the fix to AJAXLinkController - a small step from #25

[Changes to AJAXLinkController will be picked up elsewhere]

I hope I am not being too reductionist but is this issue now complete?

Me too! Thank you so much for working on this!

Now that I have a moment to think about it, I think the problem was that I was trying to autoload both the flag and the flaggable in my original design. It didn't occur to me to only autoload the flag.