#Needs issue summary update

Oh looks like other blockers are also done now, so updating again to reflect

Decrementing PP in issue title since hydration library got in

Haha, I’m with you on being efficient and avoiding unnecessary duplication of resources.

We could refactor EntityCacheTagsTestBase so that the cache bins are stored in a class member variable within the setUp() method, which is a common practice. However, I always try to be cautious about not straying too far from the original scope.

You have a lot of experience, so I’m always a bit worried about overlooking something.

Let’s see if others can share their thoughts, and we’ll decide from there. :)

Thanks so much for all the effort you put into reviewing so many (often complex) issue queues!

Think my one comment still stands but will leave in review for others. Certainly not a hill I’ll die on lol

Test coverage definitely seems to cover the basis
Left some comments in the MR.

Personally #2 seems like the least disruptive. Know on a few sites we use SSO which makes the username the email because there is no registration form. Feel option #1 would break that immediately

#3 also seems like a good approach.

Yup tried without the weight -1 and even tried

after:
- core/jquery

@smustgrave this is probably due to weight: -1 in tour module: https://git.drupalcode.org/project/tour/-/blob/2.0.x/tour.libraries.yml?...

Could you try the MR without the weight?

If tour needs to appear before something else, it might need to replace the weight declaration with the API added here.

So I tried testing this with the Tour module now in contrib. And it broke things out right, saying it couldn't find jquery even though I had it as a dependency. Tour may have a bug or it's dependencies off but this change could break existing things with no guidance where to go.

#18 ✨ Auto-save code components Active :

I see that #3499988: Editing code components was just committed so there might be some concretes ideas of what the front-end expects from the backend now.

Saving, or even dealing with any stored data was not part of that issue. We implemented creating (POST), renaming (PATCH), and deleting (DELETE) code components in ✨ Managing code components in library Active , but that's for the canonical source of the config entity, and not for the auto-saved version.

It seems like from discussion on ✨ HTTP API for code component config entities Active that we won't be implementing auto-save in in manner like content entities where the front will use something `\Drupal\experience_builder\Controller\ApiPreviewController` that returns a render markup and as a side effects does an auto-save, correct?

Instead we will be using a more direct method where the client will be sending up exactly the original and compiled js and css that it wants in the auto-save, correct?

Correct. 💯

Basically we would need a PATCH and GET(for reloading the editor) for the individual entity as saved in the AutoSaveManager

That is my understanding, too.

Converted to an MR
Still needs an IS update as there is no indication there are what has been agreed to.

Trying to get feedback.

Sorry, I completely overlooked the second point in #18.

I've implemented it, covered it with a test, and also updated the issue title. I also took the opportunity to rebase the MR.

I think I would rather disallow type-hinting with \stdClass at all to resolve this, then it not being allowed in the PhpDoc would not be a problem. Type-hinting on \stdClass seems like bad practice to me and it should be discouraged if it isn’t yet in our standards.

Anyone know what 'stdClass' was excluded?

Seems like this was written in a time when Drupal mostly used anonymous objects and we just wanted to document those as object in the PhpDoc, not as \stdClass. Actual type hints were not allowed in PHP at that time, so the question of a discrepancy between PhpDoc and in-code type hint never arose.
And we probably couldn’t think of a good reason to actually require a \stdClass object instead of any object with the required (public) properties – same as I do now.

Should this maybe be closed and another ticket opened? The change of status to "Needs review" implies that the changes are not yet released, but from what I can tell that's not true. I got the impression that 3.4 was a rollback of this, but it doesn't seem to be.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Hi

I reviewed MR!7, and confirmed phpcs, phpstan, cspell are now passed on the Gitlab CI.

Moving this to RTBC.
Thank you.

I think this will go in the opposite direction:
📌 [PP-2] Remove Implements comments once the api can pickup hook implementations from classes. Active

I have started on this

Trying figure out sizing for this issue as far as what is needed for the back-end:

I see that ✨ Editing code components Active was just committed so there might be some concretes ideas of what the front-end expects from the backend now.

It seems like from discussion on ✨ HTTP API for code component config entities Active that we won't be implementing auto-save in in manner like content entities where the front will use something `\Drupal\experience_builder\Controller\ApiPreviewController` that returns a render markup and as a side effects does an auto-save, correct?

Instead we will be using a more direct method where the client will be sending up exactly the original and compiled js and css that it wants in the auto-save, correct?

I think if that is that case the auto-saving might be able to be a simpler controller wrapped around \Drupal\experience_builder\AutoSave\AutoSaveManager

Basically we would need a PATCH and GET(for reloading the editor) for the individual entity as saved in the AutoSaveManager

does that sound correct?

I think to find the number of pages or content entities in general that use a specific component we might need 📌 Ensure querying JSON nested values when parent keys are unknown is possible in all supported databases Needs work or more likely 📌 Calculate field and component dependencies on save and store them in an easy to retrieve format Active

Also one thing to note, 📌 Calculate field and component dependencies on save and store them in an easy to retrieve format Active won't directly get us the number of instances as it would just say if it was used at least 1x on a content entity.

from the summary

Display information about pages that currently use the code component.

Is it good enough to show how many pages use the component but not how many instances of the component there actually are?

So if a component is used on 2 pages but on 1 page it is placed 3x should the number shown be 1 or 4(1 page 1x + 1 page 3x)?

Also we want show indirected uses, correct? Meaning if Component A is used on 1 page by placing the Component A directly on the page and another page by placing Component B that imports Component A, that would show 2 uses, correct?

Left comments but still appears issue summary needs updating.

> 1. Artificially change the plugin on a Views field in Views data, and check that a view using that field doesn't pick up on it

On reflection, this is probably easier.

1. Make a view with a field, say on the node title
2. Create and enable a custom module which:
-- has a Views field handler plugin which always outputs 'banana'
-- implements view data alter hook which sets this plugin for the node title field
3. Existing view should now show 'banana', but it still uses the original plugin

Checked in my local setup before and after changes. Attaching screenshots. LGTM+

Okay all resolved, including conflicts from 2.0.x branch

@avpaderno the merge is blocked by unresolved comments, but I have checked and @divyansh.gupta addressed them. I am unable to mark them as resolved myself. Can you please do?

All tests are now passing, warnings are resolved. Please review.

Switched to MR

Agreed

Thank you for searching for a test. It's quite possible that there is no test.

A little background may be helpful here: at some point in the Drupal 8? development cycle, it was decided that new commits to core require test coverage, so it is the responsibility of new commits to add test coverage if there isn't any already. That's why this issue is stuck.

The reason for requiring tests is to ensure that "fixed" things don't get broken again.

If you want to debate the rationale for this policy, please don't do it in this issue; a better place would probably be the #contrib channel in the Drupal Slack.

Hopefully someone else better informed than me can describe what kind of test should be written for this.

A bit more accurate title

Adding link to current text and the current text.

Anyone know what 'stdClass' was excluded?

@ptmkenny:
system.install @ system_requirements() is 1.5k lines of procedural goo – not exactly lending itself to a unit test. Further, the block emitting this apache-only error to apache and non-apache systems alike, is a system test.

If the hang-up is that this block of twenty lines of (test) code requires additional test coverage elsewhere in a test suite, then it would follow that a test supplying that coverage would pre-exist. I cannot find one. If someone could chime in on the rationale or logic for a test of this test, and where that test needs to live, that would be swell.

As this is merely a "bug" of omission in the first place – since system_requirements() does in fact test the server $software elsewhere for apache, but for some reason (of omission/oversight) not here – it seems to me to be an indication that there is no test coverage for this code now. Probably because it is a system test in and of itself.

Please also note that system.install @ system_requirements() around L219 runs another test for clean url (rewrite module) support, and in fact does already only conditionally proceed for apache webservers, and it conditionally does so as follows:

> && str_contains($software, 'Apache')

If there indeed does exist a separate unit or functional test supplying code coverage for this test, please point at it forthwith, as a guide.

If there is not any existing coverage of all this procedural goo, then this apache-only alarmism has been needlessly nagging maintainers of non-apache systems for years, for no good or justifiable reason, because of a bogus objection and obstruction to adding a simple && str_contains($software, 'Apache') condition to an if statement, to bypass the code for non-apache web servers.

Lower quality software lingers on and on due to insistence on what is perceived to result only in higher quality software... clearly isn't working.

The real chore that evidently is being avoided is to boil away this procedural goo and get security checks much better organized somewhere other than inline'd in system_requirements(). Forking procedurally for all these checks across all the possible web servers using if statements in a multi-thousand LoC function is very non-viable and non-testable, and as is self-evident, a maintenance headache.

So yeah, we'd need to update this: https://www.drupal.org/docs/develop/standards/php/api-documentation-and-... →

To deal with this:

object (NOT "stdClass")

The next step here is to complete the proposed resolution section.

Target branch currently set to 9.4.x

Target branch in MR needs to be updated to 11.x