Pro tip, print the list in comment 57 and hang it on your monitor ;) Saves a lot of time while getting used to it.
Yeah that should be a new issue.
Talked with catch avout a possible angle of attack which would make this viable (possobel) from a variation and perdormance perspective. Will post that sometime soon.
Almost got scared at this one, went through all function, and the count did add up, nothing missed.
One thing that has been missed though is perhaps some tests:
Drupal/FunctionalTests/Install/
has multiple tests that actually write an install function to file. Shouldn't those also be changed to add the return?
Example:
file_put_contents("$path/my_distribution.install", "<?php function my_distribution_install() {\Drupal::entityTypeManager()->getStorage('path_alias')->create(['path' => '/user/1', 'alias' => '/root-user'])->save();}");
Another lveoly clearnup. Starting to repeat myself, but changes look good. When lookin in code, this is exactly what should have been changes. See no missed code. Count didnt add up initially, but that is because of the string in "update_test_last_removed.into.yml".
Changes makes sense, had a few extra hits wehen looking around for missed ones. But those hits were in
system.install
Seems to cover all bases :) Nice one.
Checked local code, found 47 hits that need changing, code has 44. But that makes sense, becaulse of:
function twig_theme
function _drupal_maintenance_theme
function drupal_maintenance_theme
All good :)
Rebased, added experimental flag, went through threads and resolved what was needed.
This is not really something that can be installed through this project though... Is a logo needed then?
Usefull list of types with description.
https://github.com/pvdlg/conventional-changelog-metahub#commit-types
Last comment was fixed. So as per #15, this is RTBC for me. Makes quite a few things a bit more consistent. Although this test class does hurt my eyes.
This ready for review?
gábor hojtsy → credited bbrala → .
I'm so confused. I tried that earlier. But yay!
gábor hojtsy → credited bbrala → .
Yeah alt texts would be good to add. Think we should be all ready then
Left a small question.
Went through the whole thing and changes are looking good.
It does make me dream of a less magic way of testing jsonapi again. But that mountain is kinda daunting.
This issue is pretty closed. Could you an new issue about this so we can investigate?
I think * is protected right? So all branches should be protected if understand correctly.
Hmm, yeah guess so. Seems the page is a little wider that subsystem maintianer. Also this could also be used for topic maintainer. So I agree maintainer is a little better.
Thanks! I've updated the CR a little.
I prefer to test this, ive put some time into this to see if i could test it with a kernel test. Seems i can use the resolver properly, just need to do the second level of node and see if i can reproduce this. I keep getting an itch this needs testing.
Very rough work in progress.
class JsonApiDocumentTopLevelNormalizerTest extends JsonapiKernelTestBase {
use ImageFieldCreationTrait;
use ContentModerationTestTrait;
use UserCreationTrait;
public function testRelationshipVersionIncludes(){
$this->assertTrue($this->container->get('module_installer')->install(['content_moderation'], TRUE), 'Installed modules.');
$workflow = $this->createEditorialWorkflow();
$this->addEntityTypeAndBundleToWorkflow($workflow, 'node', 'article');
$this->type->getEntityType()->setHandlerClass('moderation', ModerationHandler::class);
Role::create([
'id' => RoleInterface::AUTHENTICATED_ID,
'permissions' => [
'access content',
'view any unpublished content',
'use editorial transition publish',
],
'label' => 'Anonymous',
])->save();
$this->setCurrentUser($this->user);
$this->includeResolver = $this->container->get('jsonapi.include_resolver');
$this->resourceTypeRepository = $this->container->get('jsonapi.resource_type.repository');
$this->createEntityReferenceField(
'node',
'article',
'field_entity_reference',
'Related entity',
'node',
'default',
['target_bundles' => []],
1
);
$node = Node::create([
'title' => 'original node',
'type' => 'article',
'uid' => $this->user,
'body' => [
'format' => 'plain_text',
'value' => $this->randomString(),
],
'field_tags' => [
['target_id' => $this->term1->id()],
['target_id' => $this->term2->id()],
],
'field_image' => [
[
'target_id' => $this->file->id(),
'alt' => 'test alt',
'title' => 'test title',
'width' => 10,
'height' => 11,
],
],
]);
$nodeWithReference = Node::create([
'title' => 'original node with reference',
'type' => 'article',
'uid' => $this->user,
'body' => [
'format' => 'plain_text',
'value' => $this->randomString(),
],
'field_tags' => [
['target_id' => $this->term1->id()],
['target_id' => $this->term2->id()],
],
'field_image' => [
[
'target_id' => $this->file->id(),
'alt' => 'test alt',
'title' => 'test title',
'width' => 10,
'height' => 11,
],
],
'field_entity_reference' => [
['target_id' => $node->id(), 'entity' => $node],
],
]);
$nodeWithReference->save();
$node->set('title', 'updated node');
$node->setNewRevision(TRUE);
$node->set('moderation_state', 'draft');
$node->save();
$nodeWithReference->set('title', 'updated node with reference');
$nodeWithReference->setNewRevision();
$nodeWithReference->set('moderation_state', 'draft');
$nodeWithReference->save();
$parentVid = $nodeWithReference->get('vid')->value;
$childVid = $node->get('vid')->value;
$resource_type = $this->container->get('jsonapi.resource_type.repository')->get('node', 'article');
$resource_object = ResourceObject::createFromEntity($resource_type, $nodeWithReference);
$includes = $this->includeResolver->resolve($resource_object, 'field_entity_reference');
$jsonapi_doc_object = $this
->getNormalizer()
->normalize(
new JsonApiDocumentTopLevel(new ResourceObjectData([$resource_object], 1), $includes, new LinkCollection([])),
'api_json',
[
'resource_type' => $resource_type,
'account' => NULL,
'include' => [
'field_entity_reference',
],
]
);
$normalized = $jsonapi_doc_object->getNormalization();
// Todo:
// Node 1 (static) -> reference node 2 -> reference node 3
// Update node 3 to have a working copy.
// Ask for working copy. Might need
}
}
;(
Darn. Did not check after doing the whole thing in the editor.
If noonen does I'll Retro tomorrow. Did copy paste.
Awesome!
I'll redo it.
@smustgrave can you force push again?
I just noticed also, my 11.x was polluted by a small mini commit.
Fun fact.
Issues now show the merge button in the D.O. interface. I dont dare to try it...
Back to NR after rebase.
Note; when you have done a composer install in core directory, things will fail ;p
And lol steven ;x
As per #26
Rebased and rebaselined. Will probably need to do again before commit, so here's the steps I took.
git reset --hard 96204beed3f
git rebase 11.x
composer install
vendor/bin/phpstan analyze --configuration=./core/phpstan.neon.dist --memory-limit=-1 --generate-baseline=core/.phpstan-baseline.php
git add core/.phpstan-baseline.php
git commit -m "Update baseline"
git push --force
Thanks longwave, marking as fixed.
I've uploaded all the images and made sure the structure (with step a and b) is actually readable.
I'd also add links, but I cannot since the settings pages are not available to my role. I think this is pretty good, although it good be better, I'd still say docs are updates.
One thing we might need to do is make a blogpost regarding this change, and perhaps have a better place to ask for retargeting and such. If we for example have a channel to ask for retargeting of issues we could check there and update where needed, streamlining the process.
This one needs work since mettings have not been parsed yet.
Closing, all seems good, thanks! Adding quietone for review.
Closing, all seems good as mentioned earlier. No corrections since.
Hopefully another vacation time meeting. Seems all complete and ready to go.
Very much vacation time it seems, all seems included.
I wonder if core comitters have to do something in the interface to allow all maintainers to edit or something. There are some settings around allowing edits, might be something that has a secure default.
Hehe no worries drumm. :) New territory here
So seems to work as it should yay!
Next up is getting the docs into d.o.
The source document is here so we can copy paste
https://docs.google.com/document/d/1OcYqPrtkQDlFXQs6-xSbcCsEK85In5JSxEUW...
Step 3: tried resolving a random thread (and unresolving) which worked in this mr https://git.drupalcode.org/project/drupal/-/merge_requests/1036#note_40479
Step 2: tried pushing to 11.x and got a permission denied.
Step 1: tried retargeted a random MR successfully - https://git.drupalcode.org/project/drupal/-/merge_requests/5271
That is awesome! Thank you! I will make some time, hopefully later today to have a look to this and see if this works as intended.
Hehe yeah, let's make exceptional caching into super exceptional caching haha
I think 📌 Make language switcher block cacheable Postponed could be handled without evaluating the render cache i think. Hoepfully we can get some activity there.
For this issue, we should try and keep the blockers as small as possible, seems it is a pretty short list.
This is blocked on 🌱 Optimize render caching Needs work curretnly. But im not sure thats needed. Perhaps we can find another solution that allows us to fix this issue. That would unblock #2352009: [pp-3] Bubbling of elements' max-age to the page's headers and the page cache → more, and i feel like the whole render cache issue is a bit to big and complicated.
Tried to make the comment a bit more understandable. It is still hard imo, but more clean in many ways :)
RTBC since its only a small comment change.
I was also thinking of using composer autoload to override a class to fix this, but that would mean overwriting a big class like CoreExtension, or smaller TwigFilter, but that does kinda feal like using a hammer to screw in a screw. We could though override TwigFilter, its quite a small class, but that adds complexity when updating at some point.
The solution with the drupal_spaceless would work, the issue is that the deprecation message is triggered in the ExpressionParser. This is really early in the process, and i have not found a way to really hook into that early enough.
We might be able to do some 'hackery' to adjust the specific filter in CoreExtention to not have deprecated. But i've not found a way for that.
Fair point. Will do
bramdriesen → credited bbrala → .
Rebased and small tweak to test method order and comment. All ready.
CR drafted, child issue for #6 created in 📌 aInvestigate if ComponentValidator should have soft dependency on justinrainbow/json-schema Active
bbrala → created an issue.
The intial setup from the PDF needs to be done, make the branches and tags protected (wildcards) and give maintainers the permission to allow to create.
This needs to be done by someone who has those permissions in the Drupal project. I'm more than willing to create the guide then for adding and managing the subsytem maintainers.
We are all agreed this is a good idea, so all is fine, and as soon as we fix this we can start helping out with retargeting branches etc, taking off load from the committers.
(unhappy i forgot this issue during barcelona, would've just pulled someone to do it :P)
Isnt the issue that there is extra newlines before/after/between tags, not nessesarily in the comments. The comment could still be formatted, we just need the output not to use newlines around them? We could even apply a drupal_spaceless filter on that specific node when it is visited by our nodevisitor and have that output be ok? But then again, we might as well replace apply spaceless
with apply drupal_spaceless
and start helping contrib move along.
Still though, that would not solve the fact it is deprecated and the fun contrib will have with spaceless being gone. Been looking at replacing it with out own, but the deprecation error is thrown when when the parser is doing its work. Since we cannot without to much effort get into that part of twig seems we cannot really do much.
May be a way out if we would load the druapl extentions before twig core extention, but that would mean maintaining the full contructor.
Hmmz. Can't we fix the debug output to not add newlines before/after or something? Or is there way more places where this will become an issue?
Well, it does work, but since the old one is loaded anyways before we replace it in the visitor it does actually trigger the deprecation. So not sure if we can even fully disable the deprecated one in CoreExtentions and have no error.
This might mean we need to actually replace the tag.
Ok, that approach will need more work. Was hoping for a quick one. Don't want to dive into my debugger right now and see when we can inject or override somehow.
Personally i would also rather use it only when needed like longwave suggests. There are quite a few places where it could be quite harmless to remove.
Made a quick MR to test replacing the filter.
We might be able to something like we do in 📌 Fix "Twig\Node\Expression\FilterExpression" deprecation introduced in twig/twig 3.12.0 Active then and override the spaceless filter.
This is how it is called in Twig itself:
new TwigFilter('spaceless', [self::class, 'spaceless'], ['is_safe' => ['html'], 'deprecated' => '3.12', 'deprecating_package' => 'twig/twig']),
If we need to keep it, I wonder if there is a way our then by overriding the spaceless tag so we don't trigger the deprecation. This would also be very helpfull for theres who might use it.
Draggable rows seem misaligned, more than that i cannot find.
ANother one
<div class="tabledrag-cell-content js-tabledrag-cell-content"><a href="#" title="Change order" class="tabledrag-handle js-tabledrag-handle tabledrag-handle-y"></a><div class="tabledrag-cell-content__item">Primary admin actions</div></div>
Height is wrong for the rows.
I'll do an install and fix the author. See if I can validate a few, just worried I cannot replicate all templates. Hopefully have time for that tonight.
Arg that's pretty annoying. We were hoping the fact is uses link template would remove all spaces around that.
Only one behind it it seems?
I also got nod to promise to take a look, he'll know what to expect.
DIsecting the change, you might actually be right. Instead of collecting ID's and then getting the entities we just reference the entity and create the list right away, doing away with the load multiple.
Technically its the same, there is also no risk involved with not load multiple imo since we know we have the entiy already. I cannot see how there could ever be a way to have different entities loaded with loadMultiple than we already know of in the (validated for existing) entity property.
This does assume the entity that is loaded is the correct revision, but that seems like ano brainer.
I want to RTBC, but:
- IS needs update for new approach.
- Need a change record describing the change
Added some credits. Commiter needs to decide on @mondrake
I've fixed all the issues timo and cees identified. Think every single change has now been reasoned about. Ready for review again.
Adding credit for 2 colleagues who went through it all.
Sorry, i've completely borked my response here.
I will release a new version sorry :)
I'm always a little scared about these. Won't this break others the other way around? Have other ecosystem modules been tested with this patch?
Well that feature flag does nothing unfornately. :( Ah well.
All green without most of the whitespace stuff. @longwave did raise the question if these changes could lead to layout shifts. I find that hard to know. Hopefully someone can say something smart about that.
This is eventually going to break quite a few, if not the majority of custom and contributed Drupal themes.
Is this something that automation will be able to help with? Just a couple years ago, people were explicitly advised to use the filter, as the tag was going away.
Hmmz, yeah it is gonna be quite discurptive. Basically we need to remove the spaceless tags, which would be fine. Its easy enough to make a script that removes those tags in twig templates. So in a way this could be added to update bot to fix. Although it would be something "special" since it wouldn't be rector really. So what the correct place for that is, that is something that needs some thought.
I intially throught it would also remove whitespace between html tags in the variable. Let's see how wrong i was :)
Hmm, maybe. I could check but that will take some time. Sometimes the resulsting data might also have whitespace.
I'll try and make some time to go through each and try if can be removed.
Steps:
- Find candidate
- Remove and push
- Wait for tests
- Next
Unfortunately the test doesn't want to run locally for me. :(
Tests are all green, and the only thing changes in the tests are a few newlines.
I can't RTBC anymore though. I'll go find someone for that :)
Yay got all green, now just some small optimizations to minimize the changes.
I will do an extra iteration, i think we could make tests work with only a very small change.
I've done a very small update, replacing \n with PHP_EOL and seems tests are green. This is technically fully your solution just changes the newlines.
See: https://git.drupalcode.org/project/drupal/-/merge_requests/9665/diffs?co...
Will need to check if we have adressed all comments though, i'll get backl to that, or someone else might.
Hmm, very weird.
What im thinking is it might be the fact that we have real vs character newlines. I tried to get the same error locally on that test, didnt workt.
We could try making it real newlines:
$expected = '<div class="link-item"><div class="link-url"><a href="' . Html::escape($url) . '">' . Html::escape($url_title) . '</a></div>\n </div>';
change to
$expected = '<div class="link-item"><div class="link-url"><a href="' . Html::escape($url) . '">' . Html::escape($url_title) . '</a></div>
</div>';
or
$expected = '<div class="link-item"><div class="link-url"><a href="' . Html::escape($url) . '">' . Html::escape($url_title) . "</a></div>\n </div>";
And see if that resolves it.