Brussels
Account created on 22 March 2018, about 6 years ago
  • Backend Developer at MinskyΒ  …
#

Merge Requests

More

Recent comments

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

This module doesn't work with node grants, which is what you need to have user-level access control instead of role-level access control. That means it won't work with modules like Nodeaccess. It could theoretically, but it would make the module a lot more complex and I wouldn't be able to maintain the feature since I never really use node grants myself.

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

No, it's okay to have use statements referencing non-existent classes in your code. It only causes errors when the code execution hits a reference to a non-existent class, but since drimage_imageapi_optimize_pipeline_update is never executed without the module installed this isn't a problem.

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

This MR doesn't work. willInvalidateAccessTokens is always true.

That's expected behaviour, see the comment in UserUpdateTokenInvalidationEvent:

This is TRUE by default to maintain BC. To only invalidate when access characteristics have changed, implement an event subscriber to set this to the value of ::haveUserAccessCharacteristicsChanged().

Shouldn't it be a bool for the event dispatcher to work as intended? Not a php expert but passing an empty array for a bool check on the event class seems wrong and I can't see an array length check nor how haveAccessCharacteristicsChanged is updated?

When casted to a boolean an empty array becomes FALSE an a non-empty array becomes TRUE, so this works as intended.

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

What module are you using exactly to grant your users view access per type? And what permissions? I need to know this if we want to add support for it. I could also add a view co-authored unpublished content permission to the module, but I'm not sure if that would be enough to help you here.

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

Why not keep it simple and add a hook to alter the whole image style? ImageStyleInterface already has useful helper methods like getEffects(), addImageEffect() and deleteImageEffect(), this way we can make use of those. This also opens up the possibility to do other things with the image style, like e.g. changing the name. I'll start a new branch implementing this as a proof of concept.

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

I fixed an issue where duplicate image effects were being added and I made the code a bit more readable.

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

I have a content type "visit", which view access by the author only.

How did you do this? Because the node module doesn't offer per-type 'view own' permissions, only a view own unpublished content permission. Are you using a contrib module that adds these per-type permissions?

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

We should recommend using a Permanent Cache Bin β†’ backend for the normalizer cache, to make it more efficient. This needs to be added to settings.php:

$settings['cache']['bins']['api_toolkit_normalizer'] = 'cache.backend.permanent_database';
πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

In Symfony 6 Symfony\Component\Serializer\Normalizer\NormalizerInterface got more function argument types, including one mixed type. In order to support both Symfony 5 and 6, we'll have to add those types to Drupal\api_toolkit\Normalizer\CachedNormalizer, which means that we'll have to bump the minimum PHP version to 8.0 if we want to add the mixed type. I think that's reasonable.

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

DieterHolvoet β†’ made their first commit to this issue’s fork.

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

This issue is not just about phpcs issue. As long as eslint/phpcs/phpstan issues are present and the pipeline isn't green, this isn't ready.

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

I'll go ahead and create a new release.

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

I added a first WIP version, but still needs testing. Will try to finish this ASAP.

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

Thanks for your work! I'll create a separate issue for the alter hook.

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

Okay, good to know! Thanks for the explanation.

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

I had another idea: maybe we could improve the tagify_autocomplete_matches alter hook. Currently, it's not that useful since the entity object is not passed. Only the label strings are passed, but they generally don't contain enough information to make decisions.

How about we add a tagify_autocomplete_match alter hook? We call it after $this->token->replacePlain() and we pass the $entity, $label (as reference, so we can change it) and $info_label (also as reference, so we can change it). If a hook implementation returns FALSE, the match is not included in the results.

I can implement this if you want, just let me know if you think it's a good idea.

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

The padding issues are fixed, thanks!

When I inline edit the title of an already referenced entity, after pressing enter the change is discarded.

Now the changes aren't discarded anymore after pressing enter, but they also aren't saved after submitting the form. Did it work like this before? Are you supposed to be able to change the titles of referenced entities in the Tagify widget? If so, this should be fixed.

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

Okay, I created a separate issue for the local storage discussion ( πŸ“Œ Stop storing default settings in local storage Active ) and I reworded the issue description.

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

Never mind, just noticed this has been fixed in 3.0.3.

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

After the latest updates the info tags seem to be missing right padding:

When I inline edit the title of an already referenced entity, after pressing enter the change is discarded.

I also feel like the left/right padding around the images is a little too much. How about 0.5rem on both sides, like this?

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

A comment from the maintainer in πŸ› Lazy load images not replaced correctly Active :

Regarding the "flash", we unset the src for lazy loaded images to prevent them from being loaded by the browser. It prevents duplicate images being loaded in some cases. However, if an image is in the viewport, removing the source can cause this flash. I think we might be able to figure something out to solve this.

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

I created a separate issue for the broken image flash problem: πŸ› Flash of missing image when lazy loading Active .

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

Yes, but can you share where exactly in the code this value is read? Because I can't seem to find it.

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

I'm not saying these settings shouldn't live in the preview pane. To clarify my proposition:

  • We add a section to the settings form where the site administrator can set site-wide defaults for these settings. These are stored in config.
  • We keep the settings in the preview pane, but we only store them in local settings when the editor actually changes one of these values in the preview pane. If not, we use the site-wide settings stored in config.

Another option would be to stop storing settings in local storage and to store them in the database, for all devices of the user. The fact that user-defined settings reset when switching devices could be considered unexpected. But that should probably be a separate discussion.

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

I agree @solideogloria, I have been using this patch for about 2 years now on a website where I'm an editor, and in my opinion it's a very valuable feature. It would be good to get this in the module at some point. I couldn't imagine always having to unlock the form manually.

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

@weseze okay for you if I create a 2.3.0 release?

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

Just tested this one last time on another project and changing from ImageAPI Optimize WebP β†’ to core webp works perfectly. I'll merge this.

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

Okay, in that case all good.

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

Mostly the naming, maybe it would be better to change the label to the same one that's used in the preview page: 'Open preview by default'. Looking at the code again, I can't seem to figure out where this setting is being used. Are you sure it's being used at all?

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

I think the per-device settings can still be useful, but I also think that we should only store settings on the device if the user actually changes them manually, which probably isn't going to happen that often if you're dealing with editors that aren't very tech savvy. In that case, it would be useful to be able to set good site-wide defaults.

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

You can fix the SecurityError by adding the following to your settings.php:

$settings['twig_sandbox_allowed_methods'] = [
    'id',
    'label',
    'bundle',
    'get',
    '__toString',
    'toString',
    'referencedEntities',
];
πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

My idea would be to create a select element instead of a textfield, where list all the available fields/values on the entity which we are going to use the Tagify widget, then the user could select which value want to show in the info label.

I really wouldn't do this. It would be kind of reinventing the wheel: it would introduce a lot of extra logic and we would lose a lot of flexibility that the token system provides. Tokens are the de facto way to do this kind of thing across Drupal core and contrib.

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

If we don't have it, we won't be able to add a tag with same value/name and different vocabulary.

That's not a new issue though, right? That problem is not being introduced by this new feature? In that case I would create a new issue for that and discuss it further over there, just to keep this focused.

On the other hand, regarding the use of tokens, I like the idea (don't get me wrong), but I would try to do this without depending on external modules.

Tokens are part of Drupal core. There is a contrib module named Token, which provides extra tokens that Drupal core doesn't, but the whole token concept is a Drupal core thing. You're right about the UI part, the token_tree_link theme hook, being part of the contrib module. We could place a check around it to only render it when the module is installed, since it's completely optional. The Token module is the top installed, most popular Drupal module though, so it's probably installed on most sites. I do agree that we shouldn't add it as a dependency.

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

@gxleano I had to revert the changes you did in your commit 'Avoid to remove duplicated labels from different bundle', where you changed the tagify tag value from the label to the entity ID. It broke the functionality of automatically creating new entities inline: when pasting a word in the input field or typing an unknown title and pressing enter, the tag would show 'undefined'.

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

I encountered the same issue with a token in the format [node:field_event_dates:0:value-format:date_only]. This fixes the problem. I'm using Smart Date 4.1.0-rc4.

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

In the user widget, I made the info label the line that was previously hardcoded to the user email address. Now people can choose what to show there. There's an update hook that makes sure the email will stay displayed for existing websites.

I think everything is ready now, let's start testing!

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

I renamed the 'entity label' terminology to 'info label' and I made the content of the tag dynamic. I started work on the user widget as well, but both still need some finishing touches and testing. Some use cases I already tried it out with:

Referencing events and showing the dates

Referencing music releases and showing the artists

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

About naming: we probably shouldn't call this 'entity label', since in Drupal context that usually means the title of the label. We also shouldn't use 'tag', since that's the whole feature of this module. What about 'info label'? Or any other ideas?

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

Would be nice if we could configure what exact info is shown in the label, as described in the issue description. The best way seems to me to use a text field where you can use tokens to include certain field data. Should be fairly easy to implement, I can have a look at this if you want.

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

If you have access to the entity (the node variable in node templates), you can get the co-authors with node.co_authors.referencedEntities. For example, you can get the display name of the first co-author with node.co_authors.referencedEntities|first.displayName or node.co_authors.entity.displayName.

You can also show the co-authors in the view display and access the rendered output with content.co_authors in the node template.

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

We used to recommend adding it to settings.php, but we discovered later that by using the load.environment.php method, you could use .env variables in a Drush context as well. So no, both aren't necessary. The current README.md is up to date though, it doesn't mention settings.php anymore. You should update your version of the module.

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

Maybe you could also mention that this needs to be enabled for every node type separately?

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

A content administrator logs into the site and uses same page preview. At this point localSettings stores a value for each setting (whether it is activated)

I would only store settings in local storage if the user actually changed them manually. No need to store defaults in two places, right?

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

Feel free to re-open if I'm wrong, but I don't think this is an issue anymore. We're using this module om Drupal 10 and everything is working fine. I also don't see why we would need to drop Webform 6.1 support as long as we're supporting Drupal 9.

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

Thinking about it, that still wouldn't allow people to set different displays per node type. Any chance we could just get this hook merged? I don't see the harm, we could still add another way to control this later. Alter hooks are all over Drupal.

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

A good middle way could be to add a Node label setting where you can change what's displayed using tokens. What do you think?

πŸ‡§πŸ‡ͺBelgium DieterHolvoet Brussels

I'm not sure if a hover tooltip would be a good alternative since the information wouldn't be immediately visible, and if you're using a touch device the information wouldn't be visible at all. I don't think the tag being too full would be a big concern. What exact information would be displayed would be configurable, so it's the user's choice how much they want to put in there.

I really believe this would be a good improvement, I could use it on multiple entity reference fields in a project I'm working on almost daily. Entity labels are not unique identifiers, for me it happens regularly that multiple entities have the same title and if that happens, it becomes really hard to pick the right entity from the list. Adding contextual information would help a lot.

Production build https://api.contrib.social 0.62.1