Validation of library names fails in AssetControllerBase

Even if we can never generate a URL parameter like this someone could theoretically send one so it's worth accounting for it.

Could use some test coverage - there's existing coverage of invalid asset URLs so should be straightforward to add to that.

+1 to the overall change and test coverage. LGTM!

1. In the return statement, instead of

an arbitrary string

can we please update this to

empty

/**
 * Replaces the 'include' query parameter with an empty value.
 *
 * @param string $url
 *   The source URL.
 *
 * @return string
 *   The URL with 'include' set to an empty string.
 */

2. I was just thinking if we can refactor the code duplication in test file.
A common function something like..

/**
 * Creates a URL with an empty query parameter validation.
 *
 * @param string $url
 *   The source URL.
 * @param string $parameter
 *   The query parameter to set as empty ('include' or 'exclude').
 *
 * @return string
 *   The URL with the specified parameter set to empty.
 */
protected function createUrlWithEmptyParameter(string $url, string $parameter): string {
  $url = $this->replaceGroupHash($url);
  $parts = UrlHelper::parse($url);
  $parts['query'][$parameter] = UrlHelper::compressQueryParameter('');
  
  $query = UrlHelper::buildQuery($parts['query']);
  return $this->getAbsoluteUrl($parts['path'] . '?' . $query . $fragment);
}

In terms of readability, the current code is clear and straightforward. So we can ignore this.

Thanks!

Can we clean up the steps some to be more concrete? Will help with the review.

Thank you @stborchert for providing the steps.
I verified the changes and looks good to me.

Before changes:
https://drupal11-ddev.ddev.site/sites/default/files/css/css_kRHLMdVsfEc-...

After changes:
https://drupal11-ddev.ddev.site/sites/default/files/css/css_kRHLMdVsfEc-...

Thanks!

One comment on the MR - I would have expected the validation to mean there's no need to change any code outside AssetControllerBase.

Therefore I removed the test for an empty include parameter.

I think this coverage is probably worth having - if we change the validation logic, it would prevent a possible regression.

I thought so, too. But unfortunately I ran into an error without the changes:
Exception: Warning: Undefined array key 1 Drupal\Core\Asset\LibraryDependencyResolver->doGetDependencies()() (Line: 67)

Right but this can only happen based on how it's called from AssetControllerBase, so this suggests the validation in AssetControllerBase still isn't enough?

Hi,

I rebased and tried to update the MR regarding the issue discussion to only make change in AssetControllerBase (if I understood correctly).

I kept commented changes if need to revert and still apply change in LibraryDependencyResolver.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

If I understand correctly, the changes in LibraryDependencyResolverTest and LibraryDependencyResolver only fix an error that emerged in tests but is not triggered by a real life scenarios?
So I removed the commented changes in these classes.

The Needs Review Queue Bot tested this issue. The merge request has merge conflicts and cannot be merged. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Small comment on the MR.