Arad πŸ‡·πŸ‡΄
Account created on 13 April 2006, almost 18 years ago
#

Merge Requests

More

Recent comments

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

Rerolled

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

Agree with #64. We, at European Commision, are not allowed to get code via CDN. Security is the primary concern

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

For revision support you might want to consider https://www.drupal.org/project/entity_meta_relation β†’

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

Unfortunately I'm no more interested because we switched to https://www.drupal.org/project/sitewide_alert β†’

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

@AstonVictor, welcome as co-maintainer

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

Sorry, I didn't find some time to look at your remarks. I'm trying to take a look next week

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

@vulcanr, it restores the behavior before 10.2.0 for us. Could you, please, add more details about your case?

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

@Chi, isn’t ✨ Allow end date to be optional Needs work an issue that will change this behavior?

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

Actually using that widget only makes sense when one of the range datetimes is NOW. But I have no idea if datetime range permits that

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

Wait, wait, wait… Shouldn’t this benefit from the Javascript functionality that we’ve already created in πŸ› Allow TimestampFormatter to show as a fully cacheable time difference with JS Fixed ?

IMHO, it’s the same thing, a time difference

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

@quietone, thank you for looking at this.

I’ve applied the suggestions from MR. Regarding the CR, in #109 I didn’t say that the CR is confusing. I said that it might be confusing. For me is not confusing at all but I might be wrong as I’m not a native English speaker. Then, in #111, @acbramley said that he tweaked the CR and it’s ”reading well”. What can I do more, you’re both native English speakers?

I will tentatively set back to RTBC, leaving you (or the core committers) to weight on the CR. I would fix it myself but not clear for me what’s confusing there

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

The direction set in #4 states, "- Let Views add a weight to the action plugins, perhaps first hard coded, later with a sort option in the Views interface." I read that to mean the changes to the Views interface are to be done later. That is also a suitable separation of tasks here

That was more a proposal than setting the issue’s scope. And until #152 a different approach has been considered, which we considered to be functionally wrong, for the reasons expressed in #152. Of course we can separate the UI part in a follow-up but the work is already done here and it benefited from the contributions of UX and fronted devs.

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

Actually this is a bad idea because of entity types without a langcode key.

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

If you think it solves the problem would you mind to set it as RTBC?

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

A bit annoying warning when we use unit tests.

This happens with this change?

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

Set to NW because of "Needs change record" tag

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

Thank you for catching that

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

@quietone, thank you. I’ve applied the suggestion but I don’t see the need for a CR, we’re fixing here a bug. No idea what I could write in a CR

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

As we add a new public method let’s strict type all params and the return

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

Given it has the potential to break sites, I think it's at least major

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

This change broke our site when switching to Drupal 10.2. Indeed, we've implemented hook_theme_suggestions_HOOK_alter() in a way that is not following the docs. We did:

function mytheme_suggestions_page_title_alter(array &$suggestions, array &$variables): void {
  ...
  $variables['foo'] = $bar;
}

Note that $variables is passed by reference which doesn't follow the signature of hook_theme_suggestions_HOOK_alter(). But that is still possible with hook implementation as, with hooks, there's no strong, enforcement on signature (like with interfaces/classes).

As, buildThemeHookSuggestions() doesn't pass $variables by reference, the change is lost.

I wonder if this could be considered a regression. What if there are more such unorthodox hook implementations out in the wild?

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

This needs first a decision/discussion on #93

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

claudiu.cristea β†’ changed the visibility of the branch 2320877-10.1.x to hidden.

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

claudiu.cristea β†’ changed the visibility of the branch 2320877-9.5 to hidden.

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

claudiu.cristea β†’ changed the visibility of the branch 2320877-description-required-10.0.x to hidden.

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

A new action has been introduced recently, in 79a07a4e, and that made the test fail. This is ready for review.

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

Added MR to be reviewed in IS

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

Hide attached files

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

claudiu.cristea β†’ changed the visibility of the branch 2381293-delete-action-10.1.x to hidden.

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

claudiu.cristea β†’ changed the visibility of the branch 2381293-delete-action-10.0.x to hidden.

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

claudiu.cristea β†’ changed the visibility of the branch 2381293-delete-should-not to hidden.

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

It's so confusing. Switching to Drupal 10.2 force you to reconstruct the history of this change. Can we have an unified change record merging the 2 as both changes are sequential an impacts a single update from Drupal < 10.2 to 10.2

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

This is #422 to work with Drupal 10.2 for whom might be interested

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

Re #209, #213

@smustgrave ,

Thank you for looking at this.

Meant that I couldn't reorder or remove actions with the change.

This is not about removing actions. It's only about reordering of actions on a per-view basis. I've tested again manually and everything works as expected. Also, the tests are a proof that is working correctly.

The IS is explaining all these and doesn't need any change. However, I've tweaked a little the Proposed resolution to make it more clear.

Please re-read the scope and test again. For any issue I'm available here or on Slack. Thank you.

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

Yes, the upgrade to 4.0.x is painful, sorry for that. I did my best to provide an update path even Drupal has no API to tackle such a case (changing the primary key from string to integer).

What can be done?

  • While still in 3.0x, make sure you've applied all versions until the latest from 3.0.x
  • You might consider exporting data when still on 3.0.x, then reimporting in 4.0.x

Re #7

Turned out that we have several document versions with the same timestamp suffix because they were created programmatically at the same time by a script. We have to add a custom update hook running before 9002 to solve the issue.

I was thinking that this might be possible, but 2 different documents with the same timestamp? It would be nice if we can get a patch from #7 to improve the update path

I can feel the pain but, don't forget, 4.0.x is still in alpha. That means you're using on your own risk. Everybody is welcomed to provide patches so we can make it more stable.

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

Seems to be some inconsistencies in the initial database

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

PHP 7 is end of life and not supported in 4.0.x branch. Please use a previous version that supports PHP 7

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄
πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

claudiu.cristea β†’ created an issue.

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

Let's not complicate. We're in alpha with 4. I will merge it as it is. Projects with such cases will have to deal manually with it

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

Great that you caught this. Thank you

I wonder why we don't have test coverage at least for the "Issue 2". But I will merge the fix, as is critical, and will open a followup to add coverage.

Added a comit to solve the PHPStan feedback

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

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

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

I've already hit this as in our project we're rejecting code triggering deprecations. It happened when some dependency updated Symfony from 6.3.5 to 6.4.1. Not sure whether to apply this patch or to pin Symfony

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

Why do we need to update path? Did you mean update existing roles permissions?

I was thinking that some sites might have already roles with stale permissions. But, indeed, it might be overkill to cover this scenario

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

I think we should add a PHPStan rule, @mglaman said here https://drupal.slack.com/archives/C1BMUQ9U6/p1702293217715539 that is feasible. I'm not sure I have enough skills for this. I've opened https://github.com/mglaman/phpstan-drupal/issues/686

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄
πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

claudiu.cristea β†’ created an issue.

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

@tim_dj, indeed imagecache_external_allowed_mimetypes setting is not exposed in the admin form. But that is not an effect of this issue. I think you can open a follow-up to make this setting editable

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

Thank you for catching this.

  • I think it needs a simple kernel test that created a document, sets the permission to a role. After the document is deleted, we have to assert that the permission has been removed from the role
  • Indeed, we need an update path. Try and get some inspiration from entity_legal_post_update_9002().
πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

Thank you. Adding also credits

Production build https://api.contrib.social 0.61.6-2-g546bc20