- ๐ฎ๐นItaly mondrake ๐ฎ๐น
Great to see this in.
This commit added build tests in a module, and currently
phpunit.xml.dist
is not listing them in the suites configuration.๐ Ensure run-tests.sh and PHPUnit CLI run with the same list of tests to be executed Active will fix that.
- ๐ฌ๐งUnited Kingdom catch
Moving this to fixed.
We will probably need to postpone most of the implementation issues on navigation being stable, however there might be things like moving code to the toolbar module from other modules, test clean-up etc. we can do already.
- ๐ฌ๐งUnited Kingdom catch
Opened ๐ Update to stable php-tuf releases Active .
-
alexpott โ
committed 29756947 on 11.x
Issue #3346707 by tedbow, phenaproxima, alexpott, catch, wim leers, dww...
-
alexpott โ
committed 29756947 on 11.x
- ๐ฌ๐งUnited Kingdom alexpott ๐ช๐บ๐
Crediting myself and @xjm for work on this issue.
- ๐ฌ๐งUnited Kingdom alexpott ๐ช๐บ๐
Crediting all the people who contributed to the package_manager module in contrib
- ๐ฎ๐ณIndia abhishek_virasat
alexpott โ credited abhishek_gupta1 โ .
- ๐ธ๐ฌSingapore anish.a Singapore
alexpott โ credited anish.a โ .
- ๐บ๐ธUnited States bnjmnm Ann Arbor, MI
alexpott โ credited bnjmnm โ .
- ๐บ๐ธUnited States chrisfromredfin Portland, Maine
alexpott โ credited chrisfromredfin โ .
- ๐ง๐ทBrazil diegors Campinas - Brazil
alexpott โ credited diegors โ .
- ๐บ๐ธUnited States fizcs3 Omaha, Nebraska; USA
alexpott โ credited fizcs3 โ .
- ๐ฟ๐ฆSouth Africa Idoni Hermanus
alexpott โ credited Idoni โ .
- ๐ฎ๐ณIndia immaculatexavier
alexpott โ credited immaculatexavier โ .
- ๐ต๐ฑPoland kjankowski Warsaw
alexpott โ credited kjankowski โ .
- ๐ฎ๐ณIndia kunal.sachdev
alexpott โ credited kunal.sachdev โ .
- ๐ฎ๐ณIndia narendra.rajwar27 India
alexpott โ credited narendra.rajwar27 โ .
- ๐ฎ๐ณIndia narendraR Jaipur, India
alexpott โ credited narendrar โ .
- ๐บ๐ธUnited States p.ayekumi Chicago
alexpott โ credited p.ayekumi โ .
- ๐บ๐ธUnited States percoction
alexpott โ credited percoction โ .
- ๐ฎ๐ณIndia Ranjit1032002
alexpott โ credited Ranjit1032002 โ .
- ๐ฉ๐ชGermany rkoller Nรผrnberg, Germany
alexpott โ credited rkoller โ .
- ๐บ๐ธUnited States rocketeerbkw Austin, Tx
alexpott โ credited rocketeerbkw โ .
- ๐บ๐ธUnited States Theresa.Grannum Boston
alexpott โ credited Theresa.Grannum โ .
- ๐บ๐ธUnited States tim.plunkett Philadelphia
alexpott โ credited tim.plunkett โ .
- ๐บ๐ธUnited States Webbeh Georgia, USA
alexpott โ credited Webbeh โ .
- ๐ณ๐ฟNew Zealand wiifm Wellington, NZ
alexpott โ credited wiifm โ .
- ๐ฌ๐งUnited Kingdom catch
Assuming we're able to get automatic updates in (renamed) to core, and package_manager and automatic updates reach stability in the same minor release, it might just be that the contrib module either doesn't get any update, or eventually gets an update to uninstall itself and install the renamed automatic updates version, and maybe removing itself from composer if it's able to do that. Either way it means there's nothing to do for this issue specifically in terms of release notes / CR and we can sort that out when it's all a bit closer.
- ๐ฌ๐งUnited Kingdom alexpott ๐ช๐บ๐
Discussed a bit more with @catch. I realised that there's nothing for anyone to do until we've done a few things and the release note and CR can only happen once package_manager is stable in core. This is because the package_manager module in contrib is part of https://www.drupal.org/project/automatic_updates โ - itโs not a separate module. So I think weโre going to need new releases of the contrib module. We will need a new major to remove the package_manager sub module and a new patch for the existing major/minor to declare a conflict with which ever version of core ships with a stable package manager.
- ๐ฌ๐งUnited Kingdom alexpott ๐ช๐บ๐
Discussed with @catch - we need a change record and a release note because we're moving a module in from contrib.
I've had a look at much of the code and am happy to commit this once those exist.
- ๐บ๐ธUnited States pwolanin
@catch - that sounds like a lot of thing that may not come to pass.
Also - if I'm building a data-oriented web app, I don't think I'd start from Drupal CMS, and I think there needs to still be consideration for all the use cases that want to start from a reasonably fully functional Drupal core.
So, at the very least this issue sounds premature until the Drupal CMS being stable with a good search_api recipe is implemented. At that point, it sounds like the only concern is around user confusion?
- ๐ฌ๐งUnited Kingdom catch
The issue summary says please do not push to the branch
That's outdated now the MR is canonical as off a month or so ago.
- ๐ฌ๐งUnited Kingdom alexpott ๐ช๐บ๐
The issue summary says please do not push to the branch but the way the MR is changing the dictionary is incorrect and there are whitespace errors in core/modules/package_manager/tests/fixtures/fake_site/README.md - no idea how to contribute these back properly.
- First commit to issue fork.
- ๐ฎ๐ณIndia mrinalini9 New Delhi
Fixed PHPStan issues reported by the bot in #51 but still some tests are failing that need to be fixed.
Thanks!
- First commit to issue fork.
- ๐ณ๐ฟNew Zealand quietone
@lauriii, thanks!
That is all the sign-offs needed. So, setting to RTBC.
- ๐ณ๐ฟNew Zealand quietone
@jibran, thank you.
Removing subsystem maintainer tag becuase @jibran is the maintainer of shortcut..
- ๐บ๐ธUnited States nmangold United States
Yes. Everything correct. Thanks!
Also, I think there should be a documentation change, or maybe a note in the theme settings, to explicitly tell developers the node links (including Read more) are hidden by default in the Article and Page teaser configs. Then, it is more clear how to show those links if desired, and people will know how to show/hide those links on new content types.
- ๐จ๐ฆCanada xmacinfo Canada
@nmangold Thanks for all the clarifications. I try to summarize the issue with assumptions and what needs to be done.
So:
- The node $links are already styled (but not shown to respect design decision).
- There is no need to add a theme settings.
- We currently need to create a sub-theme and we want to avoid this.
- Existing config can be used.
- Make Olivero hide (not exclude) the node Links field by default in the content type teaser
- Support already installed sites with a hook post update to hide by default the node links field
Once fixed the user stories would be:
"As a product manager for Olivero, I want the node Links field to be hidden by defautl."
"As a site builder, I want to set the node Links field to be visible via the content type teaser configuration." - ๐บ๐ธUnited States nmangold United States
I am suggesting we solve the second requirement. Simply hide the link field by default using the teaser display configurations.
By default disable "Links" on the teaser display (this is not in the current MR)
I suggested the theme setting just to make it easier for people to be aware the links are hidden by Olivero. The theme setting would ultimately control the teaser display configurations. The theme setting is not necessarily needed, and I realize now it can become out-of-sync if someone set the teaser display configuration directly.
@xmacinfo suggests using a theme setting to control the logic in the template, which would also work, but that is still bypassing the field display configurations which still makes them useless.
The problem statement here is:
"As a site builder, when I set the node Links field to be visible via the content type teaser configuration, the node links are not visible."
The node links are not visible, because Olivero forcefully excludes the field from being rendered within the Twig template. Therefore, the content type teaser configuration is not considered.
Since the requirement for Olivero is to hide the links, we can simply hide the links using the content type teaser configurations by default. This allows anyone wanting to show the links to change those configurations. Not create a subtheme and override the template. Since this was not the initial solution, we will need to modify those configurations for existing sites using an update hook, and document the change.
- ๐บ๐ธUnited States nmangold United States
@xmacinfo The template override (subtheme) is only needed currently to solve the issue. I am not suggesting we require a subtheme in the future. That defeats the purpose of this issue. Also, the links are already styled. The existing config just needs to be used.
@nod_ I am not suggesting a different view mode either. The problem here is that Olivero does not render the Links field in the node--teaser.html.twig template. I think you understand that based on your previous message "Don't explicitly exclude links from the node teaser (this is in the current MR)." However, by doing so, it makes the existing Links field display config useless. In order to show those links currently, a subtheme must be created and then the template overridden to actually render the links field. Hopefully, we are on the same page so far.
The code to which you linked is for building the Links field render array. Yes, the "Read more" link is hard-coded during the build phase to exist in that render array only within the teaser view mode. However, there is still a field display config that is (should be) used to show/hide that entire Links field.
I am suggesting that Olivero remove the links via the field display config, e.g. core.entity_view_display.node.page.teaser, after the standard install creates that config. Not via the template, which is the sledge hammer approach, which makes the links field config useless. The theme setting would just be another layer of abstraction that could toggle the visibility of the Links field within any display mode that exists at the time of it being toggled. Olivero would have the Links field hidden by default on all content type display mode configs.
I think I just realized where you are getting hung up. Maybe the theme setting should be named "Hide the node Links field." Since technically, that is the field name. "Read more" is just one of the links in the list within the render array for that field.
- ๐ซ๐ทFrance nod_ Lille
Hide 'Read more' link in teasers
That's not quite it. The design has no links at all, no comment link, no flag link, no statistics link. Being able to toggle "Read more" is not enough to solve the issue.
The problem is that Read more is hardcoded to be added only on the teaser view mode. You cannot create a different view mode that has this Read more link and use that in the views configuration instead of the teaser view mode (which would solve the problem pretty nicely without a sub-theme).
We have 2 problems:
- Read more is hardcoded to teaser view mode, and can't easily be added to a different view mode (need a configuration at the field formatter level I'd say)
- The standard install add links to the teaser that Olivero has to remove in twig to match the design
I get the frustration that a seemingly simple solution is not accepted and merged to "solve" the issue. As was said earlier, the Olivero implementation used a workaround to match the design. Sometimes we have to say stop to the duck tape and solve the root cause properly, this is one of those times.
- ๐จ๐ฆCanada xmacinfo Canada
I think the correct plan is:
1. Add a setting in Olivero with โHide 'Read more' link in teasers.โ, which would be enabled by default.
2. Change the teaser template to respect the setting.
3. Style Olivero to support the display of $link when the setting is set to show links.That way, user do not need to create a sub-theme based on Olivero.
- ๐จ๐ฆCanada xmacinfo Canada
I like the way the discussion is moving to. But the main question are:
Do we plan to implement the change in Olivero without the need to have a sub-theme?
Or, to display the links, we need to create a new sub-theme based on Olivero?
In short, a single theme (Oiivero) or two themes (Olivero and the sub-theme)?
sites will likely already have a template override
Having template overrides also mean working in another theme based on Olivero. This requires templating or hooks overrides knowledge.
Another option to expose this decision more clearly is to have a theme setting which ultimately drives the Link field display configuration, e.g. "Hide the node 'Read more' link in teasers.", which would be enabled by default.
I think this is the best solutions for Olivero users who do not need to create a sub-theme based on Olivero. With an option (on or off), a user does not need to know about templating or hooks.
However, if the settings is off or hidden by default, when switched on, will it be styled correctly? If switching it to display the the visual is not on par with the rest of Olivero, the user will be forced to create a sub-theme.
But, based on previous conversation, we might not see a styled version of the $links when a user set the setting to โDisplay the node 'Read more' link in teasersโ.
- ๐ช๐ธSpain eduardo morales alberti Spain, ๐ช๐บ
The remaining failed tests do not seem to be related to this change.
Any clue?
Failed PHPUnit:
There was 1 failure: 1) Drupal\Tests\editor\Functional\EditorSecurityTest::testEditorXssFilterOverride Behat\Mink\Exception\ExpectationException: The field "edit-body-0-value" value is "Hello, Dumbo Octopus!alert(0)", but "Hello, Dumbo Octopus!alert(0)" expected. /builds/issue/drupal-3384789/vendor/behat/mink/src/WebAssert.php:888 /builds/issue/drupal-3384789/vendor/behat/mink/src/WebAssert.php:781 /builds/issue/drupal-3384789/core/tests/Drupal/Tests/WebAssert.php:991 /builds/issue/drupal-3384789/core/modules/editor/tests/src/Functional/EditorSecurityTest.php:454 FAILURES! Tests: 3, Assertions: 131, Failures: 1. ---- Drupal\Tests\language\Functional\ConfigurableLanguageManagerTest ----
Time: 00:39.668, Memory: 8.00 MB There was 1 failure: 1) Drupal\Tests\responsive_image\FunctionalJavascript\ResponsiveImageFieldUiTest::testResponsiveImageFormatterUi Behat\Mink\Exception\ExpectationException: The string "Select a responsive image style." was not found anywhere in the HTML response of the current page. /builds/issue/drupal-3384789/vendor/behat/mink/src/WebAssert.php:888 /builds/issue/drupal-3384789/vendor/behat/mink/src/WebAssert.php:363 /builds/issue/drupal-3384789/core/tests/Drupal/Tests/WebAssert.php:558 /builds/issue/drupal-3384789/core/modules/responsive_image/tests/src/FunctionalJavascript/ResponsiveImageFieldUiTest.php:94 FAILURES! Tests: 1, Assertions: 11, Failures: 1. ---- Drupal\FunctionalJavascriptTests\Ajax\AjaxFormCacheTest ----
Failed Nightwatch:
Error โ NightwatchAssertError Timed out while waiting for element <form.system-modules [name="modules[navigation][enable]"]:disabled> to be present for 10000 milliseconds. - expected "found" but got: "not found" (10495ms) Error location: /builds/issue/drupal-3384789/core/tests/Drupal/Nightwatch/Commands/drupalInstallModule.js:39 โโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโ 37 | // Wait for the checkbox for the module to be disabled as a sign that the 38 | // module has been enabled. 39 | this.waitForElementPresent( 40 | `form.system-modules [name="modules[${module}][enable]"]:disabled`, 41 | 10000, โโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโ Wrote HTML report file to: /builds/issue/drupal-3384789/nightwatch_output/nightwatch-html-report/index.html
- ๐บ๐ธUnited States nmangold United States
@nod_ Thank you for joining the discussion, understanding the problem, and streamlining solutions.
Changing the default configuration to hide the "Links" field in teaser display appears to be the simplest compromise, and the implementation that I would have expected given the design requirement.
If you want to automate the config changes for existing sites, then we could use an update hook. The result for existing sites wanting to hide the links would be no change (no links). The result for existing sites wanting to show the links would be more involved, as those sites will likely already have a template override. If that template override considers the links field configuration, then the Links field configuration would need to be changed back after the update. If that template override does not consider the configuration, and forces the links to be rendered, then the theme developer can decide whether or not to remove their override and consider using configuration, or not. Therefore, a change record should be included which describes that scenario.
Personally, I feel the initial design decision to hide the links should have already been documented more obviously, such as in the theme README. Therefore, I think the decision to remove the links by default should added to documentation.
Another option to expose this decision more clearly is to have a theme setting which ultimately drives the Link field display configuration, e.g. "Hide the node 'Read more' link in teasers.", which would be enabled by default.
- ๐ฌ๐งUnited Kingdom catch
@pwolanin
Something like search API module is much too complex to set up and maintain if you want just a basic content search to work.
The idea is that once ๐ฑ Specification document for Advanced search in Drupal CMS Needs review is available in Drupal CMS (and once Drupal CMS is stable), then Drupal CMS will provide a pre-configured search recipe using search API and that would be the default search experience for most new users.
Drupal core won't have the standard profile any more (or not in its current form anyway), so then it becomes confusing having a search module that's not enabled in core by default and is completely replaced by Drupal CMS.
It's true that search API configuration is pretty complex, but you end up doing that anyway once you need something that core search doesn't provide out of the box, so as long as the Drupal CMS recipe gets people started sufficiently, then it should be a better situation than now.
- ๐บ๐ธUnited States pwolanin
This seems like a bad idea to me. The functionality of this module seems pretty essential for Drupal core to have a minima feature set.
- ๐ช๐ธSpain eduardo morales alberti Spain, ๐ช๐บ
Thank you! We saw that custom profiles have their user.settings, we review it!
- ๐ซ๐ทFrance nod_ Lille
Frontend framework manager here.
I agree that removing links forcibly from twig is not ideal, at the same time the design calls for no links in teasers, and that was confirmed several times over the years (#5, #61).
No links in teaser is a constrain we have to work with.
We can find solutions if we reframe the problem. What I got from the issue and all the comments is that this part is the real issue:
the Olivero theme is forcing the links to not be displayed, regardless of the field display configuration
We can totally reconcile the display and configuration:
- Don't explicitly exclude links from the node teaser (this is in the current MR)
- By default disable "Links" on the teaser display (this is not in the current MR)
That way we follow the design while making sure we didn't break expected functionality.
There are a few things to keep in mind:
- If it's disabled by default people might not be aware it's even possible to have when they change theme, making the links even less relevant maybe?
- How do we deal with existing sites? I can't support changing the default homepage of all the Olivero sites out there. There needs to be an automated way to prevent that, or at least copious release note and change record to explain to less technical users how to go back to what they had before (no-links)
We could also go another way and follow Claro/Gin: have Olivero in core and a "Better Olivero" in contrib to try more things out.
Before going further I'd like to have some ideas on how to deal with existing sites, knowing that changing all default homepages should be the very last solution.
Removing subsystem maintainer tag, we had that in #61,
Removing product manager tag, this is a technical issue, not a product issue.
Tagging for release managers for the config update part if it comes down to that. - ๐บ๐ธUnited States smustgrave
Ah are there user.settings in any profile or test modules?
- ๐ช๐ธSpain eduardo morales alberti Spain, ๐ช๐บ
@smutgrave it is happening on all types of tests not only on migrations.
Seems related to the schema validation, but the schema seems to be right./builds/issue/drupal-3384789/core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php:98 /builds/issue/drupal-3384789/vendor/symfony/event-dispatcher/EventDispatcher.php:206 /builds/issue/drupal-3384789/vendor/symfony/event-dispatcher/EventDispatcher.php:56 /builds/issue/drupal-3384789/core/lib/Drupal/Core/Config/Config.php:230 /builds/issue/drupal-3384789/core/lib/Drupal/Core/Config/ConfigInstaller.php:396 /builds/issue/drupal-3384789/core/lib/Drupal/Core/Config/ConfigInstaller.php:149 /builds/issue/drupal-3384789/core/lib/Drupal/Core/ProxyClass/Config/ConfigInstaller.php:75 /builds/issue/drupal-3384789/core/lib/Drupal/Core/Extension/ModuleInstaller.php:333 /builds/issue/drupal-3384789/core/lib/Drupal/Core/ProxyClass/Extension/ModuleInstaller.php:83 /builds/issue/drupal-3384789/core/includes/install.core.inc:1916 /builds/issue/drupal-3384789/core/includes/batch.inc:296 /builds/issue/drupal-3384789/core/includes/form.inc:977 /builds/issue/drupal-3384789/core/includes/install.core.inc:654 /builds/issue/drupal-3384789/core/includes/install.core.inc:572 /builds/issue/drupal-3384789/core/includes/install.core.inc:121 /builds/issue/drupal-3384789/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:315 /builds/issue/drupal-3384789/core/tests/Drupal/Tests/BrowserTestBase.php:546 /builds/issue/drupal-3384789/core/tests/Drupal/Tests/BrowserTestBase.php:363 /builds/issue/drupal-3384789/core/tests/Drupal/FunctionalJavascriptTests/PerformanceTestBase.php:28
- ๐บ๐ธUnited States smustgrave
Maybe something in the migration tests need to be updated to be aware of the new keys
- ๐ฌ๐งUnited Kingdom catch
I've been trying to get remove this from core since 2008.
#228594: UMN Usability: split access rules into an optional module โ .
Lets' do it.
- ๐ช๐ธSpain eduardo morales alberti Spain, ๐ช๐บ
@smustgrave What should we change on the MR?
The errors are:
Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for user.settings with the following errors: 0 [] 'cancel_method_options' is a required key.
But it is defined by the user.settings install config https://git.drupalcode.org/project/drupal/-/merge_requests/4684/diffs#1e...
cancel_method_options: user_cancel_block: true user_cancel_block_unpublish: true user_cancel_reassign: true user_cancel_delete: true
- ๐ณ๐ฟNew Zealand quietone
Thanks to dinarcon for pointing out, in Slack, the copy/paste error in my comment above.