krystalcode → credited alexpott → .
krystalcode → credited alexpott → .
krystalcode → credited alexpott → .
alexpott → created an issue. See original summary → .
I've manually tested this and we have automated coverage so I think this is looking good. I've discussed the use of DomDocument and how it fixes broken HTML with @daniel.bosen and we agree this is a price we have to pay to be able to remove links reliably.
I've addressed the review feedback and created the follow-up 📌 Simplify RemoveCheckToStringNodeVisitor when Twig 4.0 is released Active
alexpott → made their first commit to this issue’s fork.
alexpott → created an issue.
Tests are now passing on Drupal 10 and 11. Yay.
alexpott → created an issue.
Doh 10.4.x and 10.5.x have a change that 11.0.x does not. My bad. Fixed in a quick follow commit.
Committed and pushed e2a92ae2480 to 10.5.x and dd90bb9095a to 10.4.x. Thanks @spokje!
We also need to deal with recipe translation. We should see if there is an issue and if not open one. It's going to be complex.
I have one concern left - see question on MR. Anyone can rtbc once the question is answered.
Committed and pushed 35044e4519e to 11.x and 5b7419e8dfa to 11.1.x. Thanks!
FWIW we could backport this to 10.4.x / 10.5.x using https://www.drupal.org/node/3459876 →
Committed and pushed 9cf58fe16c5 to 11.x and 8fc8778c8c5 to 11.1.x and 2e7842f8e51 to 11.0.x and 72b464b48bf to 10.5.x and 35d9969bc56 to 10.4.x and b5c897f4ab1 to 10.3.x. Thanks!
Given this is an allowed change but to a majorly important form I'm only going to make this to next minor branches and not to the patch. branches (10.3.x and 11.0.x)
Committed and pushed 0b78493d29a to 11.x and 27f7b679085 to 11.1.x and 4027f70a0ec to 10.5.x and deead9fdad7 to 10.4.x. Thanks!
Committed and cherry picked back to 10.4.x
I've tried to re-create the problem on 11.x, 10.1.0 and 10.0.0 following the steps in the issue summary and it did not work. Also this is very similar to 🐛 Notice: Undefined index: #parents in a Form/FormElementHelper.php Needs work
How are we getting form elements with no parents. I think it points to unprocessed part of the form being added at the wrong point (after ) and to a larger issue.
Note I've followed the instructions in the error is no longer reproducible for a media field so I think we fixed this in a different, probably better, way.
Committed and pushed 4d7ce2a6850 to 11.x and c9820e0e613 to 11.1.x. Thanks!
Committed and pushed 6b6facdc29e to 11.x and 167dc216f33 to 11.1.x. Thanks!
poker10 → credited alexpott → .
#58 asks a good question. Here's one answer. Because if they are not required and you use the 1 time login and then save the account without entering a new password it'll save and the resulting form will have the current password field. But you don't have a current password if you've come directly from an email sent due account registration so this is then an impossible task. So I think this change makes sense.
The last fails look relevant as they are about the deprecate this introduces.
I think the MR behaviour is way better than HEAD because it is consistent. On the node edit form the time being saved is in the users timezone which makes sense. And then when a user views it they see the correct time. I think the major UX issue is that we're not showing the timezone on the node edit form as that would be very very helpful for people because maybe the node creator rkoller is very aware the site's timezone is new york and would expect node edit forms to be in the site timezone and not theirs.
catch → credited alexpott → .
Interestingly Symfony's sanitizer allows data on all media urls see https://github.com/symfony/html-sanitizer/blob/a25620fc6407e14331f3c0c56... and doesn't do anything special for SVGs. I tried inlining an SVG with JS using a data url and the JS is not executed.
One problem with the current MR is that it only accounts for src="stuff" and not src='stuff' and src=stuffnospaces which are what the two following blocks of code account for.
Also the discussion seems to be to change the list to an allow list rather than and exception list but it got rtbc'd in the current state.
Committed and pushed 524cf737ec1 to 11.x and 6d5a320db6c to 11.1.x. Thanks!
Pushed a very simple fix for people where this has been set to TRUE for whatever reason. Very odd and no idea how it worked on 10.2 but as this feature is removed in 11.x I think we can just be pragmatic and forget about this.
@rkoller thanks for the fantastic testing and yes I think discussing as a group is the best way forward.
This bug feels tricky... it you set the default relative date to +1 Saturday 13:00 whose timezone does the 13:00 apply to? The user who is creating the content - the site? I agree that there is a bug here because UTC is not the correct choice for default values. But I'm not sure that user's timezone is either. I think I would expect the site's timezone to be respected. So no matter by whom the content is created the default time is the same. Tricky. Note that if user timezones are not configurable I think this is exactly what this MR delivers.
Needs a re-roll and I think we need a CR to document the functionality as it is pretty hidden. Given the default is TRUE there's no behaviour change to new or existing sites here BUT it is nice to tell people about this functionality.
Committed and pushed 56186f9070b to 11.x and 743c2e68f70 to 11.1.x. Thanks!
Let's move the constant. This is something we need earlier than the module system being available because we download translations in the very very early installer. Therefore let's deprecate LOCALE_TRANSLATION_DEFAULT_SERVER_PATTERN and move the constant to \Drupal::TRANSLATION_DEFAULT_SERVER_PATTERN or somewhere else. Including all the locale module code throughout the installation process doesn't feel the correct approach.
Ideally this would be better fixed in calling modules. It's only triggering a deprecation so we have time to fix it. But if we are going to fix it in core then we should not do so in way that hides the warning if you don't have #text set at all. See comment on MR.
Committed and pushed ba24f059677 to 11.x and 78e8f760984 to 11.1.x and b4e268fdae4 to 11.0.x and c92e4d23b76 to 10.4.x and c26c5ee4133 to 10.3.x. Thanks!
Committed e977788 and pushed to 10.5.x. Thanks!
Backported everywhere due to this being a docs fix.
This change confuses me because it's removing @var from stuff... also if I only apply the changes to the phpcs.xml.dist and run PHPCS the other error that's found is:
> phpcs --standard=core/phpcs.xml.dist --parallel="$( (nproc || sysctl -n hw.logicalcpu || echo 4) 2>/dev/null)" -- 'core'
FILE: /Volumes/dev/drupal/core/modules/block/tests/src/Kernel/NewDefaultThemeBlocksTest.php
-------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-------------------------------------------------------------------------------------------
37 | ERROR | Missing @var tag in member variable comment
-------------------------------------------------------------------------------------------
Time: 26.72 secs; Memory: 12MB
So the rule changes are not covering the changes being made here.
@ptmkenny both of those things will be fixed by this.
Committed and pushed 1c0440c822f to 11.x and 71c6a08a300 to 11.1.x. Thanks!
@ptmkenny that module is why I discovered this issue and started to work on this :)
Committed and pushed 68013115a83 to 11.x and c3a9311a660 to 11.1.x. Thanks!
@smustgrave yes I think it is.
@sidgrafix well how does it get set to TRUE... there must be something somewhere setting it to TRUE. Is your settings.php including files from outside Drupal root?
Fixed conflicts in the baseline.
Re the 11.x vs 12.x discussion - https://www.drupal.org/about/core/policies/core-change-policies/bc-polic... → says:
Test traits and abstract base classes should generally use deprecation where possible rather than breaking backwards compatibility, but they are still considered an internal API and may change if necessary.
So I think we can do this in 11.x
@sidgrafix can you do grep -R yaml_parser_class ./ on the Drupal root directory of the server where you are experiencing the problem.
@bradjones1 the Pecl yaml parser still works - I tested this already.
@sidgrafix where is yaml_parser_class set? It should never be a boolean - that would not have worked on 10.2.x - see #31.
@sidgrafix, @emptyvoid: Can someone please run vendor/bin/drush ev "var_dump(\Drupal\Core\Site\Settings::get('yaml_parser_class'));" from the command line. We need to see what you have in that setting in order to understand why this is breaking.
This MR adds the form to the entity usage page and tab and provides a test for it. All looks good so far.
@stevenx thanks for working on this.
I think we need a section on author information that details both the interaction with a user entity reference field and how to add a custom entity reference field with vg wort participant info. These things are very related. So we need to move around stuff in the readme a bit.
I'd like to see a kernel test that proves this works for required fields... the only test we've changed is a unit test and that doesn't use a real database backend.
Merged to 4.x and a slightly different fix to 3.2.x
alexpott → created an issue.
There are definitely test usages of node_add_body_field() outside of core.
Here's examples of contrib modules that will be broken when this is released:
https://git.drupalcode.org/project/blog/-/blob/3.x/config/install/field....
https://git.drupalcode.org/project/book/-/blob/2.0.x/config/optional/fie...
- yes both modules we've removed from core (hence making this change both easier and harder) but dependencies on the current implementation of field.storage.node.body are real and this will cause disruption.
Using the alternative code search to look for dependencies on field.storage.node.body is revealing... 29 pages of results... http://codcontrib.hank.vps-private.net/search?text=-%20field.storage.nod...
alexpott → created an issue.
quietone → credited alexpott → .
@nilesh chhantbar that's not a fix for this issue - it's a delay of the problem till 11.x
If people could apply the patch and clear caches and restart the webserver this would let us know what is in yaml_parser_class setting - there must be something otherwise it would not be failing.
Well this issue is doing 📌 Stop automatic storage creation of body field Active for every non-standard install of Drupal so is likely to require changes in any other install profile or at least some interesting considerations.
Also I suspect we need to make node_add_body_field() more robust. If you call it will 'text_long' and a body storage already exists with 'text_with_summary' then it is going to use the existing storage. Also should we care if the value in $field_type is nonsense? Will the system error as expected? I guess it'll only error if it actually has to create the field storage.
I also think we need to really think about 📌 Stop automatic storage creation of body field Active - that fact that nodes get a consistent body field is used by views and other modules to allow cross node type functionality. Removing this is likely to break a few assumptions. These might be great to break and tell every one to use view modes instead but it is likely to be complex and hard.
Actually if we release an 11.x compatible version we need to say we conflict with 11.1.x because we already know we do - therefore this is going to fail hard. Not sure what to do. ick.
I think we should allow the next minor to fail... we still get the info.
alexpott → made their first commit to this issue’s fork.
I think #9 might offer a way forward that removes the eval usage and is potentially very interesting.
FWIW we cannot use the generic hook_entity_insert() here. We need to decrypt the entity at the very beginning of the calls to the hooks and hook_ENTITY_TYPE_insert is called before the generic hooks. That's why all this complexity exists in the first place. Using the eval method increases this module's compatibility with other modules massively as no module expects encrypted data :)
Re #20 ah I see we want to change the type from type: text_with_summary to type: text_long. Hmmm... not being able to rely on node creating a body field storage is likely to have interesting impacts.
I have some concerns about not shipping field.storage.node.body.yml with the node module. Why is that necessary? Also https://www.drupal.org/node/3485412 → incorrectly uses the name field.field.node.page.body.yml - that was never a part of the node module config as far as I know.
The reason the node module ships with the body field storage is so that modules can really on a body storage being available for any new node types. It allows views of nodes with different types to rely on the body field. I'm not sure why changing the formatter needs the storage change.
Added a review comment to the MR.
This looks good to go and should be merged back to 10.3.x
The test is not going to fail until we update Symfony but running locally I can confirm it fails as expected.
./vendor/bin/phpunit core/modules/views_ui/tests/src/Functional/XssTest.php
PHPUnit 10.5.38 by Sebastian Bergmann and contributors.
Runtime: PHP 8.3.12
Configuration: /Volumes/dev/sites/drupal8alt.dev/phpunit.xml
FF 2 / 2 (100%)
Time: 00:21.394, Memory: 10.00 MB
--
There were 2 failures:
1) Drupal\Tests\views_ui\Functional\XssTest::testViewsUi
Behat\Mink\Exception\ExpectationException: The string "<marquee>test</marquee>" was not found anywhere in the HTML response of the current page.
/Volumes/dev/sites/drupal8alt.dev/vendor/behat/mink/src/WebAssert.php:888
/Volumes/dev/sites/drupal8alt.dev/vendor/behat/mink/src/WebAssert.php:363
/Volumes/dev/sites/drupal8alt.dev/core/tests/Drupal/Tests/WebAssert.php:558
/Volumes/dev/sites/drupal8alt.dev/core/tests/Drupal/Tests/WebAssert.php:546
/Volumes/dev/sites/drupal8alt.dev/core/modules/views_ui/tests/src/Functional/XssTest.php:27
2) Drupal\Tests\views_ui\Functional\XssTest::testNoDoubleEscaping
Behat\Mink\Exception\ResponseTextException: The text "sa_contrib_2013_035" was not found anywhere in the text of the current page.
/Volumes/dev/sites/drupal8alt.dev/vendor/behat/mink/src/WebAssert.php:907
/Volumes/dev/sites/drupal8alt.dev/vendor/behat/mink/src/WebAssert.php:293
/Volumes/dev/sites/drupal8alt.dev/core/tests/Drupal/Tests/WebAssert.php:979
/Volumes/dev/sites/drupal8alt.dev/core/modules/views_ui/tests/src/Functional/XssTest.php:40
Note that the router entry for a view (even a REST view with a space on the end is fine because this is fixed in \Symfony\Component\Routing\Route::setPath()
Implemented @longwave's suggestion. I guess we could use a test.
I've tried to recreate this bug on Drupal 11. Here are the steps I've followed:
- Install Standard profile
- Duplicate the comment views
- Export config
- Edit the duplicate view to have paths with a space on the end
- Import config
- Visit views UI listing... no crash and can see URLs with spaces on the end in the UI. They don't work and but you can't enter these urls via the UI so I think that that is okay.
What I am missing?
We already trim the path on updating the path - see \Drupal\views\Plugin\views\display\PathPluginBase::validateOptionsForm() - so it'd be interesting to know how the space got into the path in the first place. Could people confirm if these has occurred on sites that have been migrated from Drupal 7 or do the views use a different plugin that provides a path?
Eek... but yeah this has always been a downside. This brings me back to a micro/precompiled container that that has essential services. FWIW we do have something similar already with \Drupal\Core\Config\BootstrapConfigStorageFactory...
The question here should be what config read is not cached by FileCache?
Also are we talking about parsing a single 34 MB file or 34 MB of files? Also does the server have APCu installed - if not you'll get a massive performance boost by installing this.
I think this is okay to re-introduce. I'm not convinced this should be functionality we should support forever but breaking it in 10.3 was indeed a break so we could re-introduce in the way that @longwave is suggesting. OTOH @bradjones1 could maintain and apply the patch in 📌 Allow parsing and writing PHP constants and enums in YAML files Needs work using composer patches. It'd be great to land that.
@nicxvan I think the additional test coverage added here is fine.
We have
* An argument is passed to the callbacks that indicates whether the
* transaction was successful or not.
In the docs for \Drupal\Core\Database\Transaction\TransactionManagerInterface::addPostTransactionCallback. If we're calling them after a rollback - even if the rollback cannot be performed because MySQL does not support transactional DDL then I think $success should be FALSE.
But $success is for post transaction callbacks is supposed to indicate whether it's been successful. It hasn't been even though on mysql there's been a partial commit.
Going to fix on commit as the changes a tiny and not controversial - the methods allow a NULL and the @param needs to account for that.
Committed 2277edb and pushed to 11.x. Thanks!
Committed 21b6e13 and pushed to 11.x. Thanks!
Committed and pushed 3bc3652d0fe to 11.x and 25e67bc3a55 to 10.4.x. Thanks!
@phenaproxima symfony/config was already removed. It's was not necessary so it was removed the main issue.