Valladolid
Account created on 24 April 2008, almost 16 years ago
  • PHP / Drupal Developer at LullabotΒ  …
#

Merge Requests

More

Recent comments

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Backend part is ready for review. We need now to ensure that Top Bar specific styles are moved to the submodule.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Adding reference to πŸ“Œ Convert Top Bar into an experimental submodule. Active

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

@larowlan Thank you for opening that discussion.

We need at some point to address the code duplication issue.

I'm not sure whether this is a task to work on now or when moving the module from experimental to stable.

If I understood correctly, experimental modules recommendation was to not modify other core pieces just in case they will not be finally promoted to stable. Not sure if that's not actually the case, or if this scenario could be an exception.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Since the arrival of πŸ“Œ Use PHP attributes instead of doctrine annotations Fixed adding a new arbitrary property to Block attributes is not allowed.

I'm getting this error when adding the new property to attributes:
Error: Unknown named parameter $allow_in_dashboard in ReflectionAttribute->newInstance() (line 13 of modules/contrib/dashboard/src/Plugin/Block/DashboardTextBlock.php).

Read issues comments and there are some mentions about how to alter them, but most of them leads to use and alter_hook, so defining blocks as dashboard-allowed from the attributes section does not seem to be an option, at least for not.

What do you think?

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Thank you for your review @penyaskito

Cleaned up the code a bit and fixed all the code quality and functional tests.

We might need a more in deep review if you consider it appropriate and determine whether we should refactor tests to cover both Block and Layout Builder as part of this issue or as follow-ups.

Thank you!

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

MR merged. Opened πŸ“Œ [PP] Define specific tests for disabled super user access policy Active to implement extra tests for disabled super user access policy once it is added to a stable release.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Good point!

I don't know well how to replicate the user 1 behavior in kernel tests after πŸ“Œ Add a container parameter that can remove the special behavior of UID#1 Fixed .

Actually this MR is agnostic and gives the actual responsibility to the site administrator.

If they decide to give superpowers to user 1, they'll access to all the dashboards.

If user 1 is not empowered, it would behave like any other.

Don't think we need to add extra tests to confirm the default behavior.

If you feel safer having them, we could explore how to make it possible.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Yep, I believe we will still to use this workaround - or a different one - for now.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

@m4olivei

There were some unrelated test fails due to πŸ“Œ [Meta] Fix all tests that rely on UID1's super user behavior Active . Worked on them here to avoid creating a new issue.

Fixed them, I hope we don't have more unexpected conflicts here.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

New MR adding required missing dependencies in default config

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

I think the contrib module needs to remain usable on the current release of Drupal (both for developers working on it and test run). I think for these test fixes, it's a case of leaving in the contrib module what satisfies the test runs on the current major, and addressing issues in the next major as a change required in #3439747: [No commit] Prepare for core inclusion, in the MR meant to stand in as a patch for the sync script to apply.

Good point!

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Worked on issues related to CSpell, PHPCS & PHPStan.

Some notes:

We should add to the core MR modules/navigation/assets to .cspell.json & .eslintignore ignore list

We should add exceptions for CategorizingPluginManagerTrait in .phpstan-baseline.php file

$ignoreErrors[] = [
	'message' => '#^Call to method getDefinitions\\(\\) on an unknown class Drupal\\\\Core\\\\Plugin\\\\CategorizingPluginManagerTrait\\.$#',
	'count' => 1,
	'path' => __DIR__ . '/lib/Drupal/Navigation/NavigationBlockManager.php',
];
$ignoreErrors[] = [
	'message' => '#^Call to method getSortedDefinitions\\(\\) on an unknown class Drupal\\\\Core\\\\Plugin\\\\CategorizingPluginManagerTrait\\.$#',
	'count' => 1,
	'path' => __DIR__ . '/lib/Drupal/Navigation/NavigationBlockManager.php',
];
πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

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

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

First iteration of the API definition marking some features as internal/final.

Would be great to have a second pair of eyes on this to ensure that I understood it correctly.

Thank you!

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

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

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Good job @m4olivei!

These is a great starting point for this one.

Some comments/thoughts here related your questions:

  • I think we need to mark the module as experimental in the *info.yml file
  • Should changes in *info.yml file be present only in the core's MR but not in the "official" repo?
  • From what I understand, while the module is in experimental phase, all the classes that could/should be provided by other modules like ShortcutsNavigationBlock should stay in Navigation module. They're moved when the module becomes a first class passenger.
  • An internal MR to cleanup all the code quality remaining issues would be necessary before the actual core MR would be necessary because in core MRs are deal breakers and actual PHPUnit tests are not even run if they fail. See last pipeline results.
  • Being restrict with maintaining all pipelines green before merging parallel work would be great
πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Created a initial patch for this that solves the initial issue, but could escape strings twice, like in the failing test.

Tests fails because views provided blocks have the following admin_label:

$admin_label = new TranslatableMarkup('@view: @display', ['@view' => $view->label(), '@display' => $display->display['display_title']]);

That means that malicious values in the view or display names are already escaped in advance.
However, an admin_label where the malicious part is not in a token, like the one below, should be escaped:

new TranslatableMarkup("<script>alert('XSS subject');</script>")

As far as I know, we don't have a reliable way to determine whether a string has been already escaped to determine if a second escape is necessary or not.

In the current scenario, I'm not sure whether it's better to allow XSS unsafe strings, or have double escaped ones for views. Any of the scenarios is unlikely to happen.

If someone has a better approach to solve this situation, any advice is much appreciated.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Thank you for your feedback.

Obsolete state keys are removed as part of the post_update hook.
Created change record draft: https://www.drupal.org/node/3438802 β†’

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Conflicts resolved and tests are green again. Back to NR.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Twig debug and related development toggles from "Development settings" page moved to raw key value storage as part of the MR.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

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

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

MR created to try to address this shortcut caching improvement.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Thank you!

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Sorry for being late to the party!

I would encourage to provide tests for this new feature. They'll probably be required at some point.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

MR ready for review.

Part of the test is commented because are related to shortcut module caching issues that are not fixed in 10.2.x

Once tests are run against 10.3.x or 11.x, they'll be necessary.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Branch updated with latest changes from 11.x and tests are green again.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Thank you for the heads up!

Removed the remaining BC layer deprecation notices.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Fixed. Thanks!

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Looks great!

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Thank you for the heads up.

Brought back the SDC module info file stub and added post_update hook to uninstall it.

Finally added section https://www.drupal.org/docs/core-modules-and-themes/deprecated-and-obsol... β†’ to reflect the module situation

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Fix typo in SDC obsolete version

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Add information about obsolete module SDC.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

MR created that replaces the problematic user cache context with user.permissions to make the navigation bar cacheable.

As a secondary effect, we found that cache tags were not being implemented properly because navigation bar was not including automatically new navigation blocks added.

Root cause seemed to be that navigation block list cache tag (config:navigation_block_list) was not included in the bar itself.

Once the cache tag is added, tests are back to green and 'X-Drupal-Dynamic-Cache' header is not 'UNCACHEABLE' anymore.

I believe this is ready for review now!

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

As part of the MR creation, commented the line suggested by @m4olivei in πŸ“Œ Add tests for UserNavigationBlock Needs work .

It lead to some broken tests.

Debugged it a it and found the issue in NavigationBlockCacheTest::testCachePermissions

Cache tags before the change (Test pass):
config:block_list config:navigation.navigation_block.administration_menu config:navigation.navigation_block.content_menu config:navigation.navigation_block.oxaw6wfu config:navigation.navigation_block.user config:navigation.settings config:system.menu.admin http_response navigation_block_view rendered user:2 user_view
Cache tags after the change (Test fails):
config:block_list config:navigation.navigation_block.administration_menu config:navigation.navigation_block.content_menu config:navigation.navigation_block.user config:navigation.settings config:system.menu.admin http_response navigation_block_view rendered user:2 user_view

I'm missing the cache tag related to the test navigation block added during setup().

My feeling is that reason behind failing test is that navigation bar cache tags are not properly invalidated or refreshed when a new navigation block is added.

More digging in this one will be necessary. I'll try to continue with this next if anybody finds the root cause before.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Took the freedom to create πŸ› Navigation Bar implementation makes all pages uncacheable Active to handle the caching issues and their possible ramifications.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Code looks great!

Just a added a minor code suggestion.

I would encourage to add more specific assertions to ensure cache is behaving as expected, see comment in MR.

Thank you!

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Created MR where SDC module is completely removed from the codebase and references to deprecated twig functions and LibraryDiscoveryParser service parameters BC layer are moved as well.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Created πŸ“Œ [PP-1] Remove deprecated code from shortcut module Postponed to keep track of deprecation code that might need to be removed before 11.0.0.

Also updated IS to reflect the final approach taken.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Feedback has been addressed and CR updated.

See comment on MR about the need to add static cache to getDisplayedToUser().

While we're here, would you consider appropriate a follow up issue to provide a shortcut related cache context to avoid having a cache entry per user?. In an scenario where most users have selected the same shortcut_set, most of those cache entries would be redundant.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

@alexpott New role for review including the new method ShortcutSetStorage::getDisplayedToUser() and deprecation of shortcut_current_displayed_set() & shortcut_default_set() functions.

Some followup questions.

  • We are adding new parameters to ShortcutLazyBuilders service and deprecating 2 functions. Can we have both in a single CR? Or we might need 2?
  • shortcut_current_displayed_set() implements the drupal_static() pattern. Do we need to add something special for this? It's being invoked only from ShortcutSetStorage to be refreshed once shortcut sets are being assigned/unassigned or removed.

Thank you

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Thank you for your comment.

I thought the approach would not affect he number of cache entries, given that we already had the user context, but forgot to take into account the cachetags table, where a new row per tag is added.

Based on your suggestion, this new approach invalidate the previous set cache tag when assigning a new set or unassigning the current one. This is a bit more aggressive because it invalidates some entries unnecessarily, but maintains the size of cachetags table under control.

Also , we are not introducing a BC change, so the CR created would not be necessary anymore.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Thank you for your review @alexpott.

I think all the suggested improvements have been implemented properly and the R is ready for a new round of reviews.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Created MR for this. It can be managed in parallel with πŸ“Œ Dashboard shouldn't depend on layout builder Needs work

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Updated MR with the basic functionality to manage the different logic depending on the existence of layout_builder module.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Back to active once the blocker issue has been already fixed.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

All good from my side!

Marking as RTBC.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

MR created.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Back to NR once πŸ“Œ Add tests for NavigationBlockManager class Needs review is in and the tests have been adjusted.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Feedback has been addressed and tests are green.

I think we can move it back to RTBC.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Thank you for the heads up!

Test PHPCS issues should be fixed now.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Thank you for the heads up @m4olivei!

Went ahead and fixed not only the phpcs issues related to this tests to leave the ground clean.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

MR created, but it is necessary to merge πŸ“Œ Add tests for NavigationBlockManager class Needs review first to finish the Broken plugin test.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Code looks good to me!

Left a minor comment in the MR.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Created MR for this.

Once working on it, I realized that behavior for these pages should be tabs instead of top bar options I think.

Original idea was to have only entity operations in the top bar. Tabs in these pages are not operations like when you are visiting a node, but actual links to different entity listing page.

Also, the "More actions" label does not make sense to me in this scenario.

Created a MR to exclude entity collection routes from the NavigationRenderer logic.

Please test it and we can discuss about the best approach for this issue and rephrase it if you consider it appropriate.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Good job!

As mentioned in the MR comment, I believe we need to rethink the logic to determine the defaults the module should revert to.

Let's see if @m4olivei has something in mind for this situation.

My proposal would be to have a defaults array somewhere where ,modules could register their "defaults" to revert. Taking into account that maybe not all the navigation blocks would be defined by navigation module in the future.

On the other hand, that would end up in a situation where contrib modules define their own defaults, having a long list of defautl blocks that exceeds the space available in the navigation bar.

What do you think?

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Good job in the MR!

Added some comments that should be addressed before merging this.

Thank you for your efforts.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Thank you for your review @rkoller

Could you please take a look now? Added back one of the missing cache tags.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Bump priority and assign to myself.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Thank you for the heads-up @rkoller!

Issue you mentioned is a secondary effect of the caching issues in general.

Let me rephrase this one to ensure both situations are solved here and we can close yours.

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