Unassigning for now, will pick up again next week.
mcdruid → credited longwave → .
This actually solves #2 and not the original problem described, so tagging for issue summary update.
This works locally with the image-no-example and SDDS components no longer crash for me, but some of the other SDDS components are acting strangely for me locally so this needs some more testing before it's ready to go in.
mcdruid → credited longwave → .
mcdruid → credited longwave → .
longwave → made their first commit to this issue’s fork.
This works for me on a clean install of XB 0.x-dev and SDDS - it shows up in the list, the preview works, the component renders, and can be edited:
LGTM but also I wonder if we could/should ship a minimal html.html.twig in xb_stark and negotiate the theme for the XB route, maybe not worth it though.
The preview works now, and the code changes look good (I use Tugboat on other projects so am somewhat used to configuring it)
Simplified the check for visible-when-disabled and fixed a nit in the docs, the rest of this looks ready to me.
Tested locally by moving XB into a differently named directory, the test fails before but passes with this change.
longwave → created an issue.
I've gone a bit further with the refactoring of ::getClientSideInfo(), several times we initialize one-time variables far away from where we use them so I moved them all closer to their usages, and also reordered some other things that make more sense to me. It's probably best to review individual commits and then the final result as a whole for it to make the most sense.
longwave → made their first commit to this issue’s fork.
The preview is currently failed but I get the tubgoat when I try to view the dashboard to see the error logs.
Made some minor changes and extended the test, otherwise this looks good to me.
longwave → made their first commit to this issue’s fork.
Looks good to me.
The test-only job passes without the fix, which means the test isn't demonstrating the problem described here.
This was released as part of Drupal 10.3.13. It is not applicable to later branches.
Well, I'm not convinced this is a bug at all; the option is called nl2br because it calls nl2br() on the output, and that presumably works as intended (although the additional test coverage here is nice, because currently we don't have any).
Also not sure the test exercises the bug from reading it, running the test-only job to find out.
I'm not convinced this is the intent of the feature, when it says \n I think it means a literal line break (aka the newline character) and not the actual string \n.
Glad I added the tests now. Extracted the title generation to its own static method and called it from both places instead of having to keep two strings in sync.
Added test coverage of the new flags. I'm not sure how isNew == FALSE && status == TRUE is supposed to make sense so I didn't test that specific case.
longwave → made their first commit to this issue’s fork.
If we're going to hardcode the locations would we also be hardcoding the labels? I'm not sure I see the benefit of adding flexibility for the four labels, we might as well just send only the library ID for each component then we need no metadata at all about the libraries themselves. Users could still relabel them through translations if they really wanted to.
I still think contrib or custom implementations of XB might want to add additional icons in the top bar, or regroup things in their own way, but perhaps for 1.0 we just disallow that until we have a good use case for it.
Agree with the rationale, and the test is helpful until we get an upgrade to the OpenAPI spec validator - which is somewhat outside the scope of this project.
Moved this along a bit further, fixed all phpcs/phpstan issues and fixed some of the tests. More tests to fix and Wim's review points to address still.
@lauriii Re #65 the problem with Container is that it specifies an image without an example:
# Don't put examples as this is used with and without images.
image:
$ref: json-schema-definitions://experience_builder.module/image
type: object
title: Image
but Experience Builder's image schema says the src property is required:
"image": {
"title": "image",
"type": "object",
"required": ["src"],
"properties": {
"src": {
"title": "Image URL",
"$ref": "json-schema-definitions://experience_builder.module/image-uri"
},
This causes a validation error because when you first add the component the image doesn't have a src. If an example was specified (as in the XB image component) then we use that as the fallback, but there is nothing to fall back to here either.
Other than this, the tests should be passing now and all of Wim's feedback should be implemented.
Reworked as discussed with @wim.leers, @effulgentsia, @f.mazeikis - the leading slash is not even required so any example file is relative to the SDC itself. If the file exists then the full path to it is given (e.g. for an image src), if the file doesn't exist then it is treated as relative to the root of the site. Added test cases for both these, with and without leading slashes.
Started going too far with this and almost tried to solve 📌 Split model values into resolved and raw Active as well, but that wasn't as easy as I first thought (I should have learned by now) so I rolled back those changes and concentrated on only the image problem here.
Will merge in the second branch now.
longwave → created an issue. See original summary → .
longwave → created an issue.
FWIW Wordpress.com's RSS feed does use <content:encoded> *and* wraps everything in CDATA: https://wordpress.com/blog/feed/
Neither d.o or Wordpress.com's RSS validates 100%:
https://validator.w3.org/feed/check.cgi?url=https%3A%2F%2Fwww.drupal.org...
https://validator.w3.org/feed/check.cgi?url=https%3A%2F%2Fwordpress.com%...
https://stackoverflow.com/questions/7220670/difference-between-descripti... has a summary of the problem, but there is no clear answer, because it depends on the reader. Some interpret <description> as plaintext, others as encoded HTML. Some readers also support <content:encoded> for HTML but this is not backward compatible with older readers that only support <description>. What a mess, and I don't think we can have a one-size-fits-all solution here.
#57 makes sense! Great insight to limit this to URLs - but so that it works for links as well as images - and the https://example.com/ prefix is also a fantastic idea.
I think we could just work on this in a single MR now? I don't see the point in keeping two separate ones if we are going to solve both problems here.
"The iterable of violations" doesn't make much sense, change to "the set of violations" instead, leaving at RTBC.
Given the MR fixes the issue described in the title, can we push solving the problem of handling/remapping default image URLs to a followup? The image component is no longer completely broken, just it relies on the hack in experience_builder_install() to get the default image file to the right place.
longwave → changed the visibility of the branch 3501902-replace to hidden.
@Wim you forgot to add the new FallbackPropSource file so I rewrote it from memory :D
Also fixed up the #disabled code as it wasn't quite right. Time to actually open an MR here!
Fixed the merge conflict.
If #49 is correct then I think we should roll this back, there are surely numerous hosting setups where settings.php is a symlink and there's no technical reason for this to be disallowed.
Added a test for ::filterFormValues(), added some comments and cleaned up some minor bits, this is ready for review again.
This looks ready to go!
stewsnooze → credited longwave → .
What did you do to get this error? What URL were you on? Can you provide the full stack trace, as that may help diagnose the cause.
I've added filtering to ApiLayoutController::getEntityData() to remove values from entity_form_fields that have #access => FALSE and should not be accessible by the client - this might leak information to the client even if it's not displayed in the form, so it needs to be done server side.
However this doesn't fix the error; even though field_image[0][alt] is not present in the payload any longer, the validation is still triggered - run out of time today to figure out why this is happening.
Some more nits/questions but this looks great and is nearly ready!
longwave → made their first commit to this issue’s fork.
I think this is a front end problem. The layout endpoint returns all the entity field data, but for the preview we should only be sending the input values that are actually present in the form instead of keeping all the original data and overwriting values that have changed.
So the issue that Wim raised where "Alternative text is required" appears is because in the XB preview #value for the image field is
[
'display' => false,
'description' => '',
'upload' => '',
'alt' => '',
'title' => '',
'fids' => [],
]
but if I do a normal form submission without uploading an image, many of those items - including alt, which is the problem here - are not present:
[
'fids' => [],
'display' => false,
'description' => '',
]
In managed file fields, the alt text input doesn't appear until an image has been selected for upload, at which point the value array is:
[
'alt' => '',
'fids' => [
0 => '2',
],
'display' => 1,
'description' => '',
]
I think just skipping validation errors will skip the case where an image is selected and alt text is required?
wim leers → credited longwave → .
I had to edit my organization credit here but I confirm the list of attendees.
benjifisher → credited longwave → .
Re #29 what do we do if someone enables a content type for XB, then adds a new field (or changes an existing field's widget) after the fact that doesn't meet the transform criteria?
I can reproduce the issue by installing Node 16, but we already have this in package.json:
"engines": {
"node": "^20.0.0"
},
Node 16 or 18 cause EBADENGINE errors during npm install for me, not sure what else we can do here?
What version of NodeJS are you using?
Also there is a merge conflict, some out of scope changes in composer.json/lock.
Do we need to deprecate sort()? Can we add the Collator as an optional argument to the existing method, and trigger a deprecation if it's not passed in?
Added another round of questions/nits but overall this is looking great!
I've been over the MR twice and done some manual poking around with it - no additional comments, I think this is ready to land; onwards to the next issue!
Wondering if we should drop the event subscriber entirely and try to do this in Views. https://www.rssboard.org/rss-encoding-examples documents the encoding.
In views-view-row-rss.html.twig can we just force Twig to double encode
<description>{{ description|escape }}</description>
or just add CDATA there
<description><![CDATA[{{ description }} ]]></description>
?
I was also concerned about this a while back but this is a neat solution and much simpler than I expected it to be! Two minor points, one that can wait until later, the other is just a naming issue, otherwise this looks great to me so leaving at RTBC.
Apologies for the scope creep but SortTest was failing because it required exclude-pattern to be sorted at the top level; it's nicer to be able to group these with comments so I removed that test, but I also added additional tests to ensure include-pattern and exclude-pattern inside rules are correctly sorted, and fixed all cases where this was not met.
@tedbow what happens if you use curl or a similar tool to make the same HTTP requests to the same web server?
The fix is trivial and the new test looks good to me and should ensure this feature won't regress in the future. Removing the security review tag. Thanks!
The package name is also included in the lockfile, but MR!606 did not rebuild this. MR!611 fixes this.
longwave → made their first commit to this issue’s fork.
Fixed the nits.
Yep this makes sense, nice cleanup and decoupling, and I see why it's needed for the other issue.
In #27 @lauriii states that the default image should not end up in the site's media library. It's not even guaranteed that media library will be used for media storage in the Acquia DAM case.
Instead of trying to store the default image as a media entity I'm wondering if it would be worthwhile having a custom image controller that renders default images for any component. This image can still be stored as base64 in the config entity but it otherwise doesn't need to exist on disk or as a content entity. If the prop value is empty, then we output the URL that points to this custom controller.
Optional images are still awkward though. If optional images have a default example, what do you use to tell the difference between the user wanting no image at all and the default image to be shown?
Did some more cleanup on this; I've rearranged the file and many comments to make it easier to follow. It's easier to review the new version of the file instead of trying to read the diff here.
I discovered that Coder disables SlevomatCodingStandard.TypeHints.DeclareStrictTypes.DeclareStrictTypesMissing but we want it enabled for tests, so we explicitly re-enable it here. I've done a number of further checks by breaking coding standards in files and ensuring that things are still caught as expected.
I think the <file> section is wrong as the .sh files don't seem to get checked, but that can wait for a followup.
pameeela → credited longwave → .
Recreated #7. I added with the entire Drupal and DrupalPractice sets except the rules we already exclude, removed any duplicates that are already in those sets, and added further exclusions for rules that we don't meet yet.
Given that we now pin to a specific version of Coder and also that our CI jobs are now much better than before, I think this is a better format for the file - it's easier to see which standards we don't adhere to yet, and we will never be surprised as we are in control of upgrading Coder.
Good find!
Committed and pushed daf3cd72f76 to 11.x and 22ee1f029b0 to 11.1.x and ddddd508515 to 10.5.x and 6b55b8c6d24 to 10.4.x. Thanks!
This was swappable for a long time, but this has since been removed in 📌 Drop PECL YAML library support in favor of only Symfony YAML Fixed as PECL doesn't support everything we need any more. Closing as outdated.
Something similar to this idea was implemented in #2395143: YAML parsing is very slow, cache it with FileCache →
Tagged "needs work" because this still has the "needs manual testing" tag.
I thought we could extend Drupal\Tests\system\Functional\System\HtaccessTest to try and cover this case, but given that it also needs php-fpm in a specific configuration it's not going to be very easy to recreate in CI.
While this is a security hardening, we can't entirely control what people might do with Apache directives and .htaccess; as this requires modifying the root .htaccess I am tempted to say that if you are doing that then you are on your own. But on the other hand I suppose this hardening can't harm anything if we do add it?
I am also not sure why we have two "test only" jobs defined in separate pipelines but that's out of scope here.
Eventually the "non W3C" with-chrome will go away and we will only use the Selenium drivers, so this is a good step towards that.
As far as I can tell the title is no longer "Error" but just blank when this happens.
To test this in system.routing.yml I commented out the entity list title:
entity.date_format.collection:
path: '/admin/config/regional/date-time'
defaults:
_entity_list: 'date_format'
# _title: 'Date and time formats'
requirements:
_permission: 'administer site configuration'
This results in the page having no title. We technically could default to the collection plural, but it's also just as easy to set the page title in routing.yml, and in many cases these listings are overridden by Views anyway.
Given this only affects logging to a lightly-used CI job I don't see any problems with it, we can always change it again.
longwave → made their first commit to this issue’s fork.
MR looks good to me, I guess we will find out when contrib/etc starts implementing more of these transformers.