🇭🇺Hungary @junkuncz

DONE:
- empty plugin settings forms should not see a "settingsForm" edit icon
- hide "extra fields" on EntityCustomElementsDisplayEditForm
- move some logic from buildForm() to form() (it was a debt from an earlier MR)
- a few tiny code clean-up

Okay I'll do the class merge.

Regarding the local tasks: looks like unfortunately we cannot drop out that hook at
https://git.drupalcode.org/project/views_local_tasks/-/blob/1.0.x/views_...
https://git.drupalcode.org/project/custom_elements/-/blob/3.x/modules/cu...

It's weird but probably there is no better way atm.

junkuncz → made their first commit to this issue’s fork.

Yes, added it.

See: MR 53.

Changes:

- CE display is prepared for the case when layout_builder module is enabled (hidden form + "Manage Layout" button)
- Views modes are following core approach so "default" one cannot be deleted
- Broken access handler for CE display is fixed

junkuncz → changed the visibility of the branch 3359601-re-implement-with-new-concept to hidden.

junkuncz → changed the visibility of the branch 8.x-2.x to hidden.

junkuncz → changed the visibility of the branch 3.x to hidden.

junkuncz → changed the visibility of the branch new-ce-formatter to hidden.

junkuncz → changed the visibility of the branch 3362625-customelementformatter-plugin-type to active.

junkuncz → made their first commit to this issue’s fork.

Since there are a lot of changes in an other MR: https://git.drupalcode.org/project/custom_elements/-/merge_requests/29/d...
we decided to merge this with the failing unit test and fix it here: https://www.drupal.org/project/custom_elements/issues/3362625 📌 CustomElementFormatter plugin type for entity_ce_display Active

junkuncz → created an issue.

junkuncz → created an issue.

Okay, added changes. Build is green. Let's handle the 3.x related changes in a dedicated ticket, I'll create it.

3427892-stabilise-ci-build-2.x is ready for review.

junkuncz → created an issue.

Okay it's time to do another check. (tests are fine locally)

Sure, changes have been added.
Looks like test runner is still wrong here, did a check locally and all are still green.

PHPUnit 9.6.17 by Sebastian Bergmann and contributors.

Testing /var/www/html/web/modules/contrib/custom_elements
......                                                              6 / 6 (100%)

Time: 00:28.257, Memory: 4.00 MB

OK (6 tests, 32 assertions)

@fago thanks for the review.
Accidentally I started a review instead of clicking on "Add new comment" button so that's why you didn't see my feedbacks.
All remarks should be covered now.
https://git.drupalcode.org/project/custom_elements/-/merge_requests/33#n...
There is a question here.

Thanks @fago.
Remarks are addressed, please take a look.

Running unit test with strict config schema gives the following error:

Drupal\Tests\custom_elements\Functional\CustomElementsRenderMarkupVue3Test::testNodeRendering
Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for media.type.image with the following errors: media.type.image:source_configuration.gather_exif missing schema

Tried to fix it quickly but I couldn't do, it needs more investigation and deeper understanding of schema structure.

junkuncz → created an issue.

junkuncz → made their first commit to this issue’s fork.

Are you sure we need to do anything about it? I was doing some testing and looks like entity_ce_display config object is deleted automatically during bundle delete and if I uninstall the module all related configs are removed.
Maybe we could add a tiny snippet to hook_uninstall() to make sure all entity_ce_display configs are properly deleted. Something like this:

. $storage = \Drupal::service('entity_type.manager')->getStorage('entity_ce_display');
  $configs = $storage->loadMultiple();
  if (!empty($configs)) {
    $storage->deelete($configs);
  }

What do you think?

Okay it looks good, I was confused because the testes entity did not have bundles.

I'll do more testing on the Eventsubscriber but the first round of review can be started I guess.

junkuncz → created an issue.

junkuncz → made their first commit to this issue’s fork.

Tests should be checked/adjusted? however it works for me.

Works for me as well, thanks!

I can also confirm #12.

junkuncz → created an issue.

MR has been created.

junkuncz → created an issue.

purge-2853613-12.patch patch works well and the code looks very solid however unfortunately I had no time to do a detailed review/report but definitely a good candidate to merge.

I see, okay. Good to go from my side

Adding these changes in patch format, you can quickly apply if you agree of course.

Ignore the error above, it was a configuration issue (used the same zone id twice).
The module does its jobb very well. Regarding the "Purge bunnyCDN" button I agree it's more abstract and there is already a working patch for that in Purge module: https://www.drupal.org/project/purge/issues/2853613 ✨ Invalidate a purger's cache from UI Needs review

I've discovered only two things that should be added/changes I guess:
- add purge module as a dependency to module.info.yml
- "BunnyPurger" class should implement "PurgerInterface"

Looking forward you feedback.

Hello @DieterHolvoet
Thank you for your nice contribution first.
I was doing some testing and it works quite good, except one thing which is maybe(?) my environment issue I'm not sure: when I want to purge the pull zone with ddev drush p:invalidate everything -y (parameters are already added via the config form of course) I'm getting an error which relates to Guzzle I guess Bunny purge request failed. Status code 400: Client error: `POST https://api.bunny.net/pullzone//purgeCache` resulted in a `400 Bad Request` response: {"Message":"The request is invalid."}.
However I can complete the request with a Postman call. Looks like Guzzle wants to send the request "twice" for some reason, I've attached the log (AccessKey was sanitised because of privacy)
I've found a ticket which relates to this error: https://github.com/guzzle/guzzle/issues/1432
As far as I understand when the response code is 204 and the content body is empty it throws and exception.
Could you please check it on your side as well? I'm using a strandard drupal install with Drupal core 10.2 in ddev.

Thanks!

Okay, thanks for the quick reply and explanation.

junkuncz → created an issue.

Bruhh I made some mess here.. anyways the tests are working and I also added changes by Roderik's remarks.
Ready for review now: https://git.drupalcode.org/project/lupus_decoupled/-/merge_requests/72

Looks good to me.

Okay keep hold on until https://www.drupal.org/project/lupus_decoupled/issues/3409409 🐛 Respect installations with base paths Needs review is completed. Currently we are in "dark" in the CI. (tests are green locally).

Hey @roderik,

Thank you for the nice, detailed review first.
Changes have been added based on that.

• - setUp() method is tuned to be able to create a node with schema data ("schema_web_page_description")
• - I separated the logic of the metag and schema tag to different methods
• - json response is currently converted to and associative array and it's asserted by conditions (maybe there's a better approach but I think for now it should be ok)

I look forward to your comments.

@fago thank you for your feedback.
As I see CI is using the upgraded version: https://dispatcher.drupalci.org/job/drupal8_contrib_patches/180057/console

"Installing drupal/metatag (2.0.0): Extracting archive"

Yes, you are correct both are going to work.

@useernamee thank you for the review and your remarks.

I've added a new patch including your request and I also cleaned up the module running phpcs (except README.md)

Patch has been updated. Give the chance to composer to select which version is preferred
metatag_generate_entity_all_tags() was introduced in 1.23 so that should be the minimum compatibility now.

Do not use the patch file, there is a MR instead of that.

junkuncz → created an issue.

junkuncz → created an issue.

"drupal/metatag dev-2.0.x requires php >=8.0 -> your php version (7.4.28) does not satisfy that requirement."
Looks like no chance without php8.

Upps! :( Unfortunately I have no time to fix the tests and I'm quite sure it's not because of this one line change. (Error: Call to undefined method PhpParser\Node\Name::getParts() -> seems like something is missing there).

Looks like the reason of wrong image_src attribute is an incomplete Plugin annotation. Setting the "absolute_url" = TRUE gives the ability to output() function (/src/Plugin/metatag/Tag/MetaNameBase.php line 529 in version 2.0.0) to build an absolute url.

Patch is attached.

I don't know what about og:image:url and og:image:secure_url most probably it's a different issue.