🇪🇸Spain @vidorado

I've installed an AWS S3 mock with Localstack and amazon-ec2-metadata-mock.

Then, I've been able to check that filesize() works well with a s3:// stream.

Perhaps this is not longer an issue? I don't have a real S3 bucket, maybe someone could do a real test.

Oops, sorry! :) I wasn’t sure which version to include in the message since I don’t really know when this MR will be merged. I initially put 11.0.0 and later forgot to revisit it.

So, should we always deprecate for the next minor version as an estimate?

I've created a change record and triggered a deprecation error, as it must be done according to https://www.drupal.org/node/2856615 →

Additionally, the follow-issue 📌 Remove NumberTest::testAlphadecimalToIntReturnsZeroWithNullAndEmptyString() test Active as been created in order to remove the BC test.

vidorado → created an issue.

Sorry, I didn't see @mondrake's comment. I agree that returning NULL is hiding the problem in the case of a VALID image. However, I stand by saying that zero should never be returned by getFileSize(), since its docblock states this:

/**
   * Returns the size of the image file.
   *
   * @return int|null
   *   The size of the file in bytes, or NULL if the image is invalid.
   */
  public function getFileSize();

A zero size image would never be valid, so a NULL must be returned. But in case of a valid image that we can't retrieve easily with filesize()... returning NULL is wrong, that seems clear.

I will take a look at the ImageMagick toolkit to get ideas about how to download and parse the file from S3.

Thanks all for the fedback! :)

I agree with you @mfb, and I took that into account, but it's an imposible case, as the toolkit will never validate an image as valid if the file is zero size.

Whoops, I just realized that the MR target branch was 10.x, so I created a new one targeting 11.x.

Now, $request->query is typed as InputBag, and I haven’t been able to mock it because it is a final class—not even with Prophecy. I ended up using a real InputBag, but it feels a bit 'dirty' since the unit test is relying on a class that isn’t the subject of the test.

While PHPUnit can be configured to allow mocking final classes, changing core's phpunit.xml.dist seems out of scope.

Does anyone have any ideas? I will also post a question in #testing Slack channel.

vidorado → changed the visibility of the branch 2987987-csrf-token-optional-route-params to hidden.

vidorado → changed the visibility of the branch 11.x to hidden.

I've made some manual tests, and noticed that some cases were not working, even with the proposed patch in #13.

These have been my tests:

Without the patch from #13:

$url = Url::fromRoute('test.example', ['param1' => 'value']);// --> /test/example/value <-- Works.
$url = Url::fromRoute('test.example', ['param1' => '']);// --> /test/example <-- Works.
$url = Url::fromRoute('test.example', ['param1' => NULL]); // --> /test/example <-- Works. 
$url = Url::fromRoute('test.example', []); --> /test/example <-- Does NOT work.

With the patch from #13:

$url = Url::fromRoute('test.example', ['param1' => 'value']); // --> /test/example/value <-- Works.
$url = Url::fromRoute('test.example', ['param1' => '']); // /test/example <-- Does NOT work.
$url = Url::fromRoute('test.example', ['param1' => NULL]); // --> /test/example <-- Does NOT work.
$url = Url::fromRoute('test.example', []); // --> /test/example <-- Works.

The patch in #13 has been a step towards the solution, but an additional step was needed: cleaning the optional params from the path before validating. After doing that, all four examples are working now.

This is all what has been additionally done:

• Remove the unreplaced params from the route path and the trailing slash.
• Unify with a trait the code which generates a route path replacing its param placeholders, so both classes share the same logic.
• Add a test. It's a unit test, but involving two classes: RouteProcessorCsrf and CsrfAccessCheck

vidorado → made their first commit to this issue’s fork.

Done @alexpott! :)

Thanks for the review @kim.pepper!

Mind the "null" in the @return of the docblock was added in this issue, it wasn´t already there: https://git.drupalcode.org/project/drupal/-/merge_requests/7217/diffs?co...

I believe that in the cases where a false was taken into account, it would have been with a simple truthy evaluation if($value). I think it's rare that anyone could have used if(FALSE === $value), so the NULL is probably not harming.

I've decided to adopt the solution proposed by @larowlan in #18, as it makes sense to me. Additionally, I've added tests for Breakpoint::getMediaQuery() to ensure the behavior is properly covered.

vidorado → made their first commit to this issue’s fork.

I've added a couple of tests and attempted to simulate an S3 filesystem where filesize() returns FALSE, even though the toolkit returns TRUE in parseFile()

vidorado → made their first commit to this issue’s fork.

vidorado → changed the visibility of the branch 3293135-shouldupdatethumbnail-does-not to hidden.

vidorado → changed the visibility of the branch 11.x to hidden.

@mglaman, could you confirm if this issue is still relevant? I’m unable to reproduce it.

1. I created a media item of type Image and the thumbnail displays correctly on /admin/content/media, which is the expected behavior.
2. If I update the field_media_image field (the source field), the thumbnail updates as expected on /admin/content/media. This aligns with the behavior I’ve always observed.
3. Thinking you might be referring to changing the media source field on the media type configuration, I attempted that, but it’s not allowed for an existing media type.

It looks like this restriction was introduced at least as early as 2017/12/12, so I’m guessing you’re not referring to that scenario.

I might be missing something here. Could you clarify? :D

vidorado → made their first commit to this issue’s fork.

I’ve created an MR but wasn’t able to figure out how to write the tests. The existing tests in core/modules/views/tests/src/Unit/Plugin/field/FieldPluginBaseTest.php are a bit complex for me to follow. Additionally, I’m unsure whether this can be unit tested or if it requires functional tests due to its dependency on view arguments.

I’ve also updated the steps to reproduce, as the view argument name (tid) must match the query parameter name set in the rewrite field (which was mistakenly set to term_id).

vidorado → made their first commit to this issue’s fork.

Many tests were failing because they were passing NULL or an empty string to Number::alphadecimalToInt(). So I believe we should continue accepting these two special "degenerate" values as input. Two tests have been added: one to ensure this behavior and other to verify that an exception is thrown for any other non-alphanumeric characters.

I'm not sure how to proceed if we want to provide a deprecation message. Perhaps something like this?

@trigger_error('Passing non-alphanumeric characters is deprecated in Number::alphadecimalToInt() and will be removed in Drupal 12.', E_USER_DEPRECATED);

vidorado → made their first commit to this issue’s fork.

Updated the MR and added a test

vidorado → made their first commit to this issue’s fork.

Sorry, three core tests are failing, and despite spending some time investigating, I haven’t been able to resolve it.

• Drupal\Tests\system\Functional\System\ThemeTest::testThemeSettingsRenderCacheClear
• Drupal\Tests\big_pipe\Functional\BigPipeTest::testNoJsDetection
• Drupal\Tests\path_alias\Functional\UrlAlterFunctionalTest::testUrlAlter

A screenshot for better judging:

Thanks for the thorough review, @rkoller!

I’ve just committed a proposal that I believe could be acceptable:

• I’ve moved the warning just below the initial help message, as you suggested, while keeping it prominent with a strong, so that the majority of users don’t overlook it. New users would probably notice it even without the emphasis, but experienced site builders might unconsciously skip it, so I think the emphasis is necessary.
• I’ve placed the legend in the table caption. I’m not sure if I should add aria-hidden="true", as it seems unnecessary for aural navigation.
• I’ve changed the "play button" :) to an arrow. The arrow feels more natural to me than the bullseye, and it avoids the "accordion illusion" problem.
• I’ve fine-tuned the markup so screen readers announce "Matching term: xxx" or "Non-matching term: xxx" for each result.
• I’ve highlighted only the matching terms, not their entire row, and used a success color instead of a warning color. I believe this better conveys the intended meaning.

I’ve tested the navigation with the ChromeVox Chrome add-on, and it seems accessible to me. :)

vidorado → made their first commit to this issue’s fork.

I've created a MR with the patch from #188 and a simple test for part of the changes. I couldn't figure out how to test the "argument" and "filter" view plugins that were also modified.

We also have to decide if the N-Gram solution is finally discarded in favour of this simple solution, and if this solution should be configurable (I believe it should be)

vidorado → made their first commit to this issue’s fork.

Tests added! :)

Thanks for the review @nikolay

I've made some changes to address the issues identified by @gaelg, but I can't resolve the threads on git.drupalcode.org; I don't see any button to do so...

Thanks for the review @gaelg! It is very useful to polish the feature.

I will get into it tomorrow 😊

Wow! This is an awesome UX review! I will get to it in a couple of days.

Thanks a lot!

@smustgrave Thank you for the rebase and the review!

The highlight purpose was to allow the user identify the real search results. It can be odd if you make a search for a string and you see multiple terms that don't match your query (i.e. the neccesary-to-put parents of the terms that did match).

Thanks also for posting this into #ux, let's see what the experts think about it :)

@r-mo, ¿Could you tell us if this is solved by using this module?

https://www.drupal.org/project/static_asset_cache_buster →

If so, ¿Should we get along with this issue queue and commit this into core, or should we rely on that module?

I'll change the issue status to "Needs review" in order to any maintainer could answer to that, after your feedback.

This is a little old issue, Neither recall I the problem very well, but I can tell you that I've used middays in non-time dates to make less probable a day change due to a final local timezone applied. Maybe that was also your thought...

@prashant.c already noticed the textb9x overlapping issue in #48, and I found that the problem was hard to solve and out of scope.

I don't see the problem with the warning you squared in one of your screenshots. Could you explain it better?

Thanks!

That's just what I was seeing :smile: Other tests don't inherit docs! :)

But I've added the statement, of course! All ready then!

Just a C-Spell issue. Resolved! :)

I've disabled term reordering while filtering, as I believe this is safer to avoid strange things happening due to weight changes over a partial table.

I've also added a message to warn the user about the reordering not being available.

@malcomio In my opinion, sorting is a bit tricky since it interferes with term weights. In fact, I haven’t thoroughly tested what happens when you rearrange filtered terms.

Perhaps we should disable table dragging while filtering, which could provide a safer way to manage the order. :)

I'll look into it.

Thanks for the helpful UX review, @prashant.c.

I've added a placeholder, which makes the filter's purpose clearer.

I couldn't figure out how to add a margin between the textfield and the filter button in a theme-agnostic way. It might be a theme-related issue. I've seen other pages in the configuration with the same issue.

vidorado → changed the visibility of the branch 11.x to hidden.

vidorado → changed the visibility of the branch 2975863-8.9.x-taxonomy-overview-list to hidden.

vidorado → made their first commit to this issue’s fork.

@Jelle_S It will select the second one, as it is the more specific one (determined by being the longer string)

I'm not 100% sure, but a purely CSS solution would require using Unicode characters in the content CSS attribute of the ::after and ::before pseudo-elements.

Regarding your question about the "wrapper around the link and icon": if I understand correctly, you are asking whether a wrapper is being used or not, correct? I have conducted some tests and found that it is not. The icon is simply placed before the <a> element.

Nevermind, I've tested the solution in #11 and it works well for me, even though I have tricky CSS in my anchor, with a span inside that has a display: block. So I've added it to the MR.

vidorado → made their first commit to this issue’s fork.

@smustgrave Ready for review! :)

vidorado → made their first commit to this issue’s fork.

@smustgrave, I figured the problem out :)

All tests passing on Drupalcode!

Sorry for the flooding! (my first MR 😅)

I didn't managed to resolve a PHPUnit test issue in the pipeline. In my local environment all tests are passing.

vidorado → changed the visibility of the branch 8.x-1.x to hidden.

Think about it like "another config to allow the user to solve its needs". Maybe my use case is a bit kind of weird but, in my opinion, It should not harm giving the user a complete include/exclude logic toolset.

Whatever you decide, I'm sure it will be ok :)

We had a content type whose links must open in a new tab, even though they would be considered internal, as they pointed to the same site pages.

The case this feature solved for us was forcing some links to behave as external, even when they were not.

I hope it makes sense to you now! :)

For anyone also on this issue... I would like to share my experience after some playing with it :)

I realized that working with classes in SDC property definitions is not as practical as I first thought. The magic of SDC is in creating components "on the fly" in yor theme's twig files, and that is not possible if you have to provide a Class argument to a component.

If we would have a way of creating class objects from plain objects or arrays directly from twig (with a function, filter, whatever...) this could all be possible but, without that, I think is better to go along with simple types (string, boolean, integer...) and enums. We will have a little bit of repetition if we, for example, use the same enum in several places, but it is manageable.

If, like me, you also are lacking autocompletion when defining component properties, maybe a plugin for PHPStorm/VSCode could be developed, that was aware of SDC definitions.

@geek-merlin was there and checked that it worked, but only for first level property types. For property items type, it doesn't work yet. I've applied the changes in MR4871 but it still doesn't work for me.

@prudloff, Could you review again your changes against the latest Drupal core (10.2.6 at the moment) to check if anything has changed in ComponentValidator::nullifyClassPropsSchema() that makes your changes don't work?

While I was fighting with it by myself, I changed the method ComponentValidator::getClassProps() to retrieve also the classes of item types. There is, maybe it could help.

  $types = is_string($type) ? [$type] : $type;

changed to

if (is_array($type) && in_array('array', $type) && ($itemsType = $prop_def['items']['type'] ?? NULL)) {
  $types = is_string($itemsType) ? [$itemsType] : $itemsType;
}
else {
  $types = is_string($type) ? [$type] : $type;
}

but that doesn't make it work either.

The code in this Validator is very hard for me to understand, it has a very strong array manipulation with reducers and the like...

The patch in #129 commented the line

// $rule->pattern = mb_strtolower(str_replace('/*', '*', $rule->pattern));

And two thing where stripped out:

- The replacing of '/*' to '*'.
- But ALSO making the rule case insensitive.

I attach a new patch to restore the latter.

@Nicxvan, Maybe it is related with this issue and you have, like me, a patch applied?

https://www.drupal.org/project/purge/issues/2921309 →

https://www.drupal.org/files/issues/2921309-17-port-purge-drush-9.patch →

I can confirm what Azinck says in #106

Another test that really should be added is to test the interaction with paths that have similarities to the redirected path. For instance, patch 101 introduced a significant bug with this code: $rule->pattern = mb_strtolower(str_replace('/*', '*', $rule->pattern)); -- that causes redirect loops if you create a redirect like:
/foo/* => /foobar/*
That's due to the fact that /foo/* pattern is now really being interpreted as /foo*...which is just not correct. But the tests didn't catch that error.

A redirect /r/* has become /r* which is a CRITICAL bug

The patch from #123 has the problematic line commented:
// $rule->pattern = mb_strtolower(str_replace('/*', '*', $rule->pattern));

And now that bug has gone.

Anybody remembers what was the purpose of that str_replace()? It does not make sense for me right now...

Rerolled patch for Entity Reference Revisions 8.x-1.11

You are right Berdir, my mistake! My last patch doesn't apply.

I didn't realized there were other patches in my composer.json that were altering the code before. I must always test a patch in isolation, lesson learnt.

And about the issue flow, thanks, I will do that the next time. The "Drupal 10 Compatibility" title led me to wrongly staying here, sorry!

Sorry for re-opening a two-years-old issue, but I've found a query without ->accessCheck() and I thought that it fit here.

@e0ipso, this code is, on some parts, pretty much crafted to my use case with Date Range fields, buy I would love to evolve it to a more general and consistent way to be really useful :)

I'm waiting for your thoughts.

Attaching patch to ease the testing process for everyone :)

vidorado → created an issue.

Patch in #6 works!! :)

vidorado → created an issue.

Patch in #2 tested and working! :)

Agg, thanks @spuky, what a mistake!

vidorado → created an issue.

vidorado → created an issue.

Patch #3 works here too!!

vidorado → created an issue.

#7 Solved my problem. The latest revision was not being picked in a multi-language scenario, and now it is, correctly in all languages the entity is translated into.