Finland
Account created on 20 December 2010, about 13 years ago
  • Staff Software Engineer in the Drupal Acceleration Team at Acquiaย  โ€ฆ
#

Merge Requests

More

Recent comments

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

Looks like core/modules/filter/filter.filter_html.admin.js has dependency on it, but it's not declaring it's dependencies correctly... ๐Ÿ˜…

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

It doesn't look like the MR is implementing the feedback from #97.

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

Maybe we should consider opening a follow-up for some next steps. I think the first next step would be to always add the new menu item to the end of the list. Another next step would be trying to come up with a better UI for users to select where the new menu link should be added.

I think this also needs a change record.

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

Looks like the bug is fixed and #8 has been addressed. Would be nice for sure to have a common component between these two use cases but it doesn't have to happen here.

Committed 89fde7a and pushed to 11.x. Also cherry-picked to 10.2.x. Thanks!

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

We opened #3391592: Define goverance changes that the core committers are responsibile for โ†’ for discussing how the committers make changes to the governance. I reverted the governance row change from the PACSI matrix to address #84. This also makes it consistent with the "How are decisions made?" list which states the following:

If the change affects the governance, philosophy, goals, principles, or nature of the project, it must be approved by the Project Lead.

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

For me the bigger issue is that this ends up happening so late in the process, and only when some kind of release deadline was approaching - i.e. pretty much every minor release, someone whether a core committer or other contributor will start pinging me about some kind of release or framework manager sign-off I often hadn't heard about before.

My own experience has been that generally people really start pushing for sign-offs when there's already risk of it being too late for the decision to happen. I've suffered from this too. I remember thinking that the committers probably have more important challenges to concern themselves with. To address this, we need more proactive involvement from the committers to enable decision making early in the process. We need a culture where it's clear that the committers want to be involved and that reaching out to them with a low bar early on is not just allowed, but expected.

On top of that, I think it would be helpful to have a discussion on how different roles would like to sequence decisions, e.g. at which stage of the process do we start becoming uncomfortable to mark a module stable. I don't know that we should set hard limits but it would be helpful to hear what these timelines look like because now the only timeline we track is the actual deadline.

I'm comfortable with Product Managers being the tie breaker in the case of a stalemate but it would be good if their thought process/research/reasoning was made public. With any luck that will change the stalemate and prevent needing to use a tie-break option.

I believe product managers should conduct their work transparently, and give other roles an equal opportunity to be heard. I believe the current change already documents these both: "While product managers lead the decision-making, their approach should be transparent and include everyone's input.". Do you think that sufficiently covers it, or do we need to make some changes to that?

I don't mind if product managers get the final casting vote when there is a deadlock, but this leaves open the possibility of product managers overruling all (or even most of) the framework and release managers about a particular decision, which in such a case would make release and framework management impossible to carry out.

In practice, it would not be easy for product managers to make a decision where all of the framework and release managers are against the decision. Many of our decision require commitment from all of the roles, and this is hard to achieve in a situation where everyone in those roles is against the decision. For this reason, this would not be effective, even if the governance technically allowed it.

Thinking about that example a bit more, it seems that a situation where the release managers and framework managers are against product managers would likely point to a bigger problem. In this situation, it seems more appropriate to have discussion about the vision and the strategy. In particular if the vision and the strategy are clear enough, if there's alignment on them, and if the team is committed to executing them.

To me this seems more relevant to scenarios where there are some people vetoing a decision, but there's at least some support for the decision from all roles. These are the scenarios where this could help us avoid postponed and diluted decisions.

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

I am moving the issue back to NW for Comment #13. Losing the notice about media and image fields is a regression. I am adding a note to the issue summary so that we do not lose track of this regression.

I'm not 100% sure the message is needed for the media type at least when not editing fields for media entity. Maybe on this issue we move the message to the field config edit form and discuss in a follow-up if we should not display it when editing fields for other entity types than media.

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

We need a follow-up for #40. Including that in the proposed resolution too.

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

Updating the proposal based on the latest discussions on the issue. I'm removing the "needs product manager review", but I'm adding the "needs framework manager" review.

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

Committed 77888cf and pushed to 11.x. Also cherry-picked to 10.2.x as a non-disruptive bug fix. Thanks!

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

Committed 869929d and pushed to 11.x. Also cherry-picked to 10.2.x. Thanks!

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

Added a comment in the MR

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

Committed 8088712 and pushed to 11.x. Thanks!

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland
๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

Thank you @frankdesign and @mrshowerman! Based on #164 and #165 +1 on the feature from me. I haven't looked at the actual code though.

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

Committed c3c9dd1 and pushed to 11.x. Thanks!

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

lauriii โ†’ made their first commit to this issueโ€™s fork.

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

Committed e16d8e0 and pushed to 11.x. Cherry-picked to 10.2.x as a non-disruptive bug fix to Claro. Thanks!

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

Based on the number of followers and participants, this seems like a feature that several folks would like to have. I'm not sure I understand when is this feature used and I didn't find any discussion about this from the thread. It would be helpful if folks could comment here / update the issue summary to explain what are the use cases for this feature. ๐Ÿ˜Š

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

Committed 1e8ae69 and pushed to 11.x. Cherry-picked to 10.2.x as a non-disruptive a11y bug fix. Thanks!

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

I'm wondering if we should special case the default modes and allow creating that in case that it doesn't exist? It looks like it's not guaranteed that all entity types create the default view mode for all bundles. Node module is ensuring that the default modes are created in node_add_body_field.

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

I got stressed at first because I thought I had somehow accidentally changed the button id in ๐Ÿ“Œ Update CKEditor 5 to 40.2.0 RTBC ๐Ÿ˜… Luckily it looks like it was just a left-over change from an approach that I reverted there. This seems like a normal bug given that you can see what the button is by hovering it.

Committed 49175bf and pushed to 11.x. Also cherry-picked to 10.2.x. Thanks!

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

The overview pages are generated based on menus, so unless the module is doing something really unique (e.g. with preprocess), all of the links appearing in the overview pages should appear in the menu. See \Drupal\system\SystemManager::getAdminBlock if you're curious how the overview pages are generated.

However, section pages built using other mechanisms like local tasks (i.e. Block Layout) or tables is still a common pattern, so we need to have some level of support for getting to the parent link. For example here's the same problem with Paragraphs module:

Out of the box, Drupal doesn't provide links under the Block Layout link. I'm wondering which module is providing those links? ๐Ÿค”

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

I'm not sure why I was not seeing the setting being saved in my previous testing. I tested this both with existing fields and new fields and it seems that the value gets saved correctly with the MR. FWIW, it was working in the UI in all of my previous tests so maybe I was not looking at the right value with xdebug.

So for what I'm concerned, the MR should be ready. We can work on moving the settings to the storage part of the form in a follow-up issue.

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

lauriii โ†’ made their first commit to this issueโ€™s fork.

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

@johnpicozzi do you have a specific reason why you'd like to open "Structure" page? Wouldn't you be able to access all of the links directly from the navigation? I can definitely see that not being able to access "Block layout" is a problem.

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland
๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

The MR should be ready to go. I opened a core issue for this problem ๐Ÿ› Field config edit form handles form state changes inconsistently Active but it probably makes sense to merge the fix here before the core issue gets fixed.

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

lauriii โ†’ created an issue.

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

I think we should update the issue summary to remove the parts that have been addressed by ๐Ÿ“Œ Make field selection a two-step form Fixed .

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

Opened MR because it looks like this module is already using GitlabCI ๐Ÿคฉ

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

It looks like field permissions is triggering save of the field storage in an entity builder. Now that the field storage may be edited in the field config form, this could cause other undesired side-effects.

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

Is there more information about what happened in the PHP error logs?

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

lauriii โ†’ created an issue.

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

Pushed commit that fixes #70.

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

Committed 0f1bc00 and pushed to 11.x. Thanks!

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

We should probably update the issue summary since it specifically asks the following questions at the moment:

Is TUF integration an Alpha, Beta, or Stable requirement for core inclusion?
Is TUF integration requirement the same for Package Manager, Project Browser, and Automatic Updates?

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

Changed the approach so that it doesn't require an empty translatable string

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

Why are people constantly bringing up different versions of this and not instead spending that time getting the module ready to commit? How many hours have been spent talking about it in the past year?

I believe this conversation has kept going because there hasn't been a decision on this. From my end, the conversation has mostly ended because I have been tired of having the discussion, but not because I believed there was an actual decision.

Bringing the beta requirement into this issue is just making what could have been a much easier decision more difficult - why not have opened a new issue for this instead of completely changing the scope of this one?

I wanted to bring the beta issue here, because I thought it was better to be transparent about our thoughts regarding the beta requirements before making a decision regarding the alpha requirements. Bringing it up later could have come up as a surprise, which could have made it look like we're trying to get rid of the requirement altogether by removing it from the requirements whenever we're deciding on requirements for the current milestone at hand.

I'm happy to see that by bringing this up, we surfaced some blockers to removing TUF from the beta requirements that seems like could be addressed. I'm fine with moving that discussion to a follow-up issue.

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

I agree that the complexity introduced by not having TUF enabled by beta Automatic Updates is not ideal. Thanks for explaining in detail the complexity involved with that. However, this risk would be only valid if we didn't have TUF-enabled with the Automatic Updates in the next release. Not having Automatic Updates in the next release seems even worse because it would mean that we would have to wait at least until 2025 for stable Automatic Updates. Potentially even until June 2025, if we follow our current policy where experimental module statuses are only changed in minor releases.

It sounds like we could try to minimize the risk related to enabling TUF after it has been shipped by introducing a beta blocker to evaluate feasibility of #30 if TUF has not been enabled. We could decide on the approach closer to the release depending on the status of TUF. If #30 isn't feasible, then Automatic Updates could not reach beta until TUF has been enabled.

The ideal scenario is still that by 10.3.0 we have TUF enabled Automatic Updates. Unless #30 introduces too much complexity, having Automatic Updates without TUF would be still better than not having Automatic Updates at all. This would enable us to start testing TUF whenever it's done, potentially well before 10.4.0. This way Automatic Updates could potentially even reach stable in 10.4.0, and be enabled by default.

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

Discussed with @Gรกbor Hojtsy and we agreed that from our perspective TUF should be at maximum a stable blocker for both Automatic Updates and Package Manager. We acknowledge the integral role of TUF in enhancing security for automated updates, especially when updates are done at scale. However, not having TUF should not prevent us from getting the Automatic Updates in front of the core maintainers, core developers, and early testers. We believe we should do this as soon as possible because now the module is being built in its own silo.

We understand the need for thorough testing before enabling Automatic Updates by default, including testing updates with TUF. However, tying its development to alpha/beta milestones seems counterproductive. We can see that there are benefits to being able to do a holistic security review but it sounds like a lot of this is already possible before the TUF has been deployed on d.o packagist in production per #24.

Concerning the adoption of Automatic Updates, it's clear that a majority of the user base requires a high level of confidence before enabling such features in production. While the contrib module already offers automatic updates, its limited adoption hints at the community's cautious stance on this. In our research, we found that users prefer to wait until Automatic Updates has proven its reliability over time before starting to use it themself.

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

Looks like generally all of the maintainers are +1 on this. I think we still need 10.2.x backport but the 11.x MR is ready.

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

I tested the MR manually both with and without vertical tabs. I personally find reading something like the body content easier with the 832px width. This is pushing the CKEditor content to be quite a lot wider from that of native text editors. I don't want to hold this change on that but just wanted to bring that to attention. I do like the vertical tabs fix because it at least allows improving that a little but. I think we need a follow-up because vertical tabs are currently not usable on screens below ~1300px on the node form.

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

Committed da8a336 and pushed to 11.x. Thanks!

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

Moving completed issues to the completed section.

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

I agree that having an upgrade path would be useful but there's going to be some complexity with that. Drupal has decided to take the trade-off that we make it easy for site builders to customize the admin UI with the down side that it's significantly harder for us to make improvements to the experience. We could potentially check if the view was unmodified and apply the upgrade path in that case but that's for the upgrade path to define.

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

I completely agree that those things should be changed. But in the interest of keeping the MR easy to review, I suggest doing them in a follow-up issue. The scope of this issue is to break the form up into two steps. As much as possible, it should keep the existing structure.

+1 for a follow-up. Do you think we could add a brief comment explaining what each of these values are for the meanwhile?

As I moved code around, I resisted the temptation to make changes like this, or to fix uses of the global t() function. There is a lot of code cleanup that I would like to do.

I'm fine with not fixing the calls to t() if that's preferred ๐Ÿ˜‡. I proposed the change for the lines that we were already modifying.

I did not realize that ๐Ÿ“Œ Provide alter for the field type UI definitions Fixed had been created as a separate issue. But that is exactly the sort of small, well-scoped issue that I have been advocating.

There's also ๐Ÿ“Œ Create a new OpenModalDialogWithUrl command Active now ๐Ÿ‘

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

#140: Did you re-install Drupal to test this? This is only available for new installations at the moment. #139 tagged this for a follow-up to discuss if we should provide a upgrade path for this. Generally we don't provide upgrade paths for the admin view changes but this seems like a significant enough UX improvement that this might warrant one.

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

Added comment to the MR regarding a todo comment. Also the MR needs a rebase now that ๐Ÿ“Œ Provide alter for the field type UI definitions Fixed landed.

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

CR updates have been done.

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

Renamed the alter hook based on feedback from @alexpott.

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

Addressed #5 and tests are passing.

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

Added a draft CR.

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

I think we should create change record for this and update https://www.drupal.org/node/3364263 โ†’ to reference it.

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

I don't think we need a follow-up. We could probably remove it but at the same time it could be helpful to have the plugin and the custom insert button already available in case that CKEditor team decides to make more BC breaking changes to their image insert button.

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

Committed 31a6cdf and pushed to 11.x. Also cherry-picked to 10.2.x. Thanks!

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

Tests are passing. I think we could potentially get rid of the DrupalInsertImage since it isn't doing anything at the moment. This could be disruptive because we need to update existing config, but there could be also code referring to the toolbar button, e.g. other plugins could depend on it with a condition. I don't think we should block the update with that.

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

Couldn't we just move the logic from template_preprocess_views_view to stable9_preprocess_views_view?

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

lauriii โ†’ made their first commit to this issueโ€™s fork.

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

Can we move the BC class to Stable 9? That's the standard approach and allows us to get rid of it once we replace Stable 9.

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

Looks like the failing test may be a regression in the config validation system. Opened separate issue with failing test: ๐Ÿ› \Drupal\Core\Form\ConfigTarget is not fully serializable Active .

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

It looks like this may have caused the \Drupal\Core\Form\ConfigTarget to not be fully serializable: ๐Ÿ› \Drupal\Core\Form\ConfigTarget is not fully serializable Active .

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

We already have a follow-up for refining the descriptions: ๐Ÿ“Œ Refine field descriptions Active . I added the full stop for bullet points as a consideration there.

This seems like a small enough issue to not require full on usability testing. Removing the tag and moving to RTBC since it would be nice to get this done before 10.2.0.

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

We should decide also whether to use full stop for bullet points. This was raised as feedback in ๐Ÿ“Œ Date range should be in the date_time category Needs work .

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