Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
> 'title' => t('Flag %flag_title', [
With some flag names this isn't very clear.
I think we can do better. Something like 'Flag ENTITY TYPE PLURAL with the FLAG flag' maybe?
Comments
Comment #2
Brolad CreditAttribution: Brolad at Skilld commentedAgree with author, it's much better
Comment #3
joachim CreditAttribution: joachim as a volunteer commentedThanks for the patch!
However, this needs a bit more work:
getFlaggableEntityTypeId() get you an entity ID which is a machine name, not a human-readable label.
You need to pass that entity ID to one of the entity type managers (I forget which) and get the plural label.
For example, instead of 'node' you want 'Content'; instead of 'taxonomy_term', 'Taxonomy terms'.
Also, nitpick, but use %flag_entity_type as your placeholder, rather than %flag_entity.
Comment #4
joachim CreditAttribution: joachim as a volunteer commentedPostponing this on #2716613: Action access control should be on a per FlagType basis. .
Comment #5
joachim CreditAttribution: joachim as a volunteer commentedComment #6
Brolad CreditAttribution: Brolad at Skilld commentedComment #8
Brolad CreditAttribution: Brolad at Skilld commentedComment #10
martin107 CreditAttribution: martin107 as a volunteer commentedI think the test failures here are a sign of broken tests - so that should be fixed first
As a minor review nit-pick
\Drupal::entityTypeManager()
should be replaced by injecting the appropriate service into the constructor.
Comment #11
martin107 CreditAttribution: martin107 as a volunteer commentedInjected the EntityTypeManager
restesting against 8.5.x cos 8.6.x is a little broken at the moment.