Per Alex in #15, I have reduced the scope a bit to still only support the canonical link template, but to allow URL options (certain ones) to be passed in. There is test coverage that absolute and relative URLs are supported via the absolute option, which defaults to TRUE in other to be broadly compatible with uri and uri-reference formats.
What else needs to be done here?
phenaproxima → made their first commit to this issue’s fork.
Ah yes, thanks for reminding me. Implemented! Assigning to @pameeela for manual testing and review.
Fixed the remaining things and added ignore missing to fail gracefully.
Tested this and confirmed that the UX is now correct, and the props seem to behave as expected. Assigning to Bálint for review.
I'm manually testing every single altered prop to ensure they have the desired UX.
As far as I can tell, all feedback is addressed and missing test coverage has been implemented.
That looks shippable to me. The update path is a tad squirrelly and I was initially going to ask why we need a whole-ass event subscriber for it, but then I realized it's because it would be possible to set it back to an empty string at any point, even after the update path did its job. But how nice it is to dump a bunch of workaround code from SchemaTestTrait!
This should be sorted out before Mercury is stable. Tagging accordingly.
I think this is a stable blocker, since we want images to be rendered consistently in Mercury-based themes. Because Drupal CMS's stable release requires Mercury to be stable as well, it's a stable blocker for Drupal CMS too.
This blocks stable releases of Mercury and Drupal CMS.
In reviewing the MR, I have to admit I don't really understand why it's implemented how it is. My main question is this:
Why are the event information stored in local config? That makes it not updatable, at all, without an upgrade path (i.e. hook_update_N).
Is the idea here to have a feed of events that are coming up in your area? If so, is there already a page somewhere we could link to? If so, that would be a lot easier -- we could define a couple of links that get added into the administrative navigation upon install, for example, much the way that the module already does. That'd be quick and easy, and it looks like it'd fulfill the use case (maybe an "Events near you" link and a "Find your local user group" link).
UX-wise, we could even create a new subsection of the navigation, called "Community" or something like that, to house those links. That might be an even better use of this module, and a nice way to implement the ideas here. It might help, at this point, to ask a UX person for some guidance.
Pam also confirmed for me in Slack that she would have no particular objection to adding this to Drupal CMS, once it's implemented properly in this module (however we go about doing that). That would be still need to be in a separate issue with her explicit sign-off.
I have the most experience with this subsystem so I'll deal with this one.
Yeah, that's what I would do as well.
In fact, this issue collides with that critical fairly head-on, so let's block this on that one.
This is a question for Bálint or Lanny, but as a matter of future-proofing, is it possible we could configure Tailwind such that it never scans for utility class names in *.yml files? That would be a fairly strong guardrail against this regressing.
Well that was a slow, boring few hours but I think I got all the hg: stuff out of the components.
This did call for some refactoring in the Twig templates; I think I got it right but I would love review from Bálint and Lanny to make sure that a) my approach was kosher, and b) that I didn't miss anything obvious.
Unfortunately we cannot merge this as-is because it further entrenches the release-blocking anti-pattern that we're trying to stamp out over in
🐛
Do not use Tailwind CSS class names in prop enum values
Active
. So this needs to be refactored a bit to at least remove the Tailwind classes from *.component.yml files.
Emphasizing that we cannot release another tag of Mercury, at any stability, until this is corrected.
Oh shit. 😱
I'm glad that not too many components are affected by this, because this is a release-blocking critical show-stopper. This is problematic enough to make me "ground stop" even another alpha tag of Mercury, let alone a stable release of Drupal CMS, until it is fixed.
Bálint is correct -- component props' metadata, like enum information, is directly tied into the Canvas data model, at the very least in the form of the version hash. Tying styling information into the data model is an expressway to hell and needs to be fixed.
Everything done in this MR seems reasonable to me, but it does need a quick manual test from Pam, in the context of Byte, to make sure we didn't accidentally break any styling.
RTBC from a code review standpoint, but should not be committed until Pam confirms it's good.
There's no need for explicit test coverage here; the removal of created from the default content fixture is sufficient, because ContentExportTest already asserts that the exported content is identical to what's in the fixture.
Great! This will need a follow-up issue, then, specifically to add this block to Drupal CMS's dashboard -- that decision is up to Pam, so the best choice would be to open the issue without a merge request until she explicitly signs off.
If we're going to do this, it should not be happening in Drupal CMS Helper.
That module is meant specifically to polyfill missing pieces from core and contrib; providing a block is completely out of its scope.
I would instead suggest that we add it to Drupal Association Extras → , which is precisely the kind of place that a block like this would make sense.
The main benefit is that it won't be up to Pam whether to add this block or not; instead, the decision would be made by @hestenet and DA folks. Pam would decide whether Drupal CMS should expose the block (i.e., by adding Drupal Association Extras as a dependency).
So I'm going to move this issue over that module's issue queue, then assign to @hestenet for input on whether he thinks this feature should be accepted.
Missed a spot here; if the default content system is fully polyfilled, then there is no need to register the event subscriber conditionally. And there is a flaw that needs to be corrected -- the event subscriber will break, outright, if Canvas isn't enabled.
I would recommend we hold off on this until the product owner (@pameeela) approves doing it. Otherwise we're doing work that may not be merged. :(
No complaints here. Started a merge train for 8.x-2.x. Thanks!
The test was breaking because, for some reason, the ordering changed due to the rename. This is most likely due to some implicit, unstable sort somewhere which is not being normalized.
But more to the point, it took over an hour for me to puzzle out how to fix the test (by adjusting the order to match what is actually coming from the controller). This is because we are asserting against giant array dumps, which is flat-out unacceptable if it's going to cause this kind of development pain.
This isn't the issue to deal with it, but I just want to reiterate that this kind of testing is very bad for Canvas's long-term maintainability, and undoing this bad test architecture should be prioritized.
+1 RTBC. The changes make sense to me and it's wise that these subjective decisions are confined to PropSourceSuggester.
We could use the existing DataDefinition::setSetting(). Despite not being an interface method, core relies on it in many, many places.
Wow, what a test! That's really thorough coverage and much appreciated. I have a couple of minor stylistic nits (feel free to ignore those if you don't agree with them), but my bigger concern is the tests being willing to bail out if the CI environment is misconfigured. That means the tests will not be reliable -- they might just be skipped even if something is broken. If the CI environment is misconfigured in such a way that it breaks these tests -- which is unlikely -- then that means we have to fix something about the CI environment, not the test.
This is certainly an improvement worth making, but two things stand in its way:
- Merge conflicts.
- A performance fix like this will definitely need some kind of test coverage so that we don't accidentally regress it. It seems to me that a kernel test might be the best fit here?
Merge train started for 8.x-2.x. Thanks!
I don't think this needs a change record. Forms are internal. I think this fix makes a ton of sense, and I confirmed that the test (with a few changes) passes when merged with 8.x-2.x.
phenaproxima → made their first commit to this issue’s fork.
PHPStan jobs appear to be passing, except for the oldest one due to some PHPUnit backwards compatibility nonsense: https://git.drupalcode.org/project/crop/-/pipelines/657037
So I guess this is basically fixed?
I see no harm in shipping the logo that is also used on the project page. Committed and pushed to 8.x-2.x. Thanks!
Those categories make sense to me. Fixed!
This makes sense to me, and seems safe since as @amateescu points out, the handler is only ever instantiated by Workspaces. Committed and pushed to 8.x-2.x. Thanks!
I'm not finding any usages of strpos() left in the tests directory, so I guess this one is fixed by default...?
This makes a ton of sense. Merge train started to 8.x-2.x, thanks!
Temporarily assigning to @pameeela for a decision whether we will in fact add Canvas as a low-level dependency (to drupal_cms_content_type_base).
Adding #3557220: Add tbachert/spi to the default list of trusted Composer plugins in Package Manager → as a related issue, since that would also help here.
OK -- I decided to make link_rel non-nullable and default to canonical. That avoids the painful update path, in this issue at least.
Merged into 1.2.x and cherry-picked to 2.x and 2.0.x. I wrote some test coverage. Still needs a change record, though.
I like how luxuriously clear and detailed the test coverage is -- those comments make it much more accessible to the casual reader.
Why hold this up any further?
Merged into 2.x and cherry-picked to 2.0.x.
Another legitimate bug found, and clearly reported, by @pritish.kumar! Fix was tested and confirmed to work, so I'll land this shortly.
phenaproxima → made their first commit to this issue’s fork.
This is temporarily blocked by 🐛 The ? optional syntax for config actions breaks recipe validation Active .
Read through this and I don't have much left to complain about. The test coverage is stunningly thorough, although there are a few places where I think it could be tightened up.
Once my feedback is addressed, this can go straight to RTBC as far as I'm concerned.
Merged into 2.x and cherry-picked to 1.2.x and 2.0.x. Nice find, nice fix!
On second thought, why postpone? This will fix itself as soon as @zengenuity or @mandclu merges the related issue and tags a new release. No need to wait on that. Besides, this MR fixes other bugs; re-titling to reflect that.
This is a pretty sensitive and careful refactoring so I'd rather handle it myself.
phenaproxima → made their first commit to this issue’s fork.
Confirmed that this is due to an upstream bug in Easy Email. Let's postpone this issue on that fix.
I'm about 99% sure this is caused by a bug in Easy Email: #3557052: MailManager isn't really a proper decorator, and it fails to discover attribute-based mail plugins →
I'll try to get that fixed and released, which would correct this issue in short order.
Tests are failing here, unfortunately.
Alright, well, I'm reproducing this and two other bugs:
- core.menu.static_menu_link_overrides needs to be handled with config actions.
- Same with user.role.anonymous and user.role.authenticated, which need to use the grantPermissions action.
Is there a way for us to kernel test this condition?
Merged into 2.x and ported to 2.0.x and 1.2.x.
There really should be a recursive-sort-by-key method in core, dammit. Opened ✨ Add a recursive sort-by-key function to SortArray Active for that.
I have one stylistic suggestion, but it's a nit. The fix (and excellent comment) otherwise make sense to me.
I had an idea for how to make this more flexible -- if needed, and in a follow-up.
We could support a subkey option, like this:
setComponentThirdPartySetting:
component: user_picture
provider: my_module
subkey: image_styles
settings:
- image_style_name1
- image_style_name2
That would be the equivalent of doing:
$component['third_party_settings']['my_module']['image_styles'] = ['image_style_name1', 'image_style_name2'];
...rather than fully overwriting all settings for that specific third-party provider, which is what the MR currently does.
To be clear, I am proposing we proceed with the current implementation, then add this subkey stuff if the need arises for it.
Don't rename the "content" link to "CMS"! 📌 Round 1.3: Test user understanding of layout and labels for content section in Drupal CMS to align with Drupal Canvas Active revealed that this is confusing for people.
OK, I think I've addressed the feedback. It's Sunday evening here, so take it away, Australians.
Merged into 2.x and cherry-picked to 2.0.x, then partially ported to 1.2.x. Thanks!