🇦🇺Australia @nigelcunningham

nigelcunningham → created an issue.

nigelcunningham → created an issue.

nigelcunningham → changed the visibility of the branch 2987537-custom-menu-link-11.x to active.

nigelcunningham → changed the visibility of the branch 2987537-custom-menu-link-10x to hidden.

nigelcunningham → changed the visibility of the branch 2987537-custom-menu-link-11.x to hidden.

FWIW, I've traced an issue in which saving a page was leading to a WSOD to stuff that seems to be related to this issue. In the site, which also uses menu_item_extras (though I don't think that's relevant), we were seeing an attempt at updating a node resulting in:

Drupal\Core\Entity\EntityStorageException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'bundle' cannot be null: UPDATE "menu_link_content" SET "revision_id"=:db_update_placeholder_0, "bundle"=:db_update_placeholder_1, "uuid"=:db_update_placeholder_2, "langcode"=:db_update_placeholder_3 WHERE "id" = :db_condition_placeholder_0; Array ( [:db_update_placeholder_0] => 56 [:db_update_placeholder_1] => [:db_update_placeholder_2] => 3e5d49ee-3087-4200-a9bc-caf63f09dd06 [:db_update_placeholder_3] => en [:db_condition_placeholder_0] => 18 ) in Drupal\Core\Entity\Sql\SqlContentEntityStorage->save() (line 817 of /var/www/html/docroot/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php).

Looking at what mapToStorageRecord in SqlContentEntityStorage is doing, I saw that $entity->bundle->value == 'main' but the code is trying to retrieve the bundle value from $entity->bundle->target_id. The difference seems to be due to ContentEntityBase having the following logic:

    if ($entity_type->hasKey('bundle')) {
      if ($bundle_entity_type_id = $entity_type->getBundleEntityType()) {
        $fields[$entity_type->getKey('bundle')] = BaseFieldDefinition::create('entity_reference')
          ->setLabel($entity_type->getBundleLabel())
          ->setSetting('target_type', $bundle_entity_type_id)
          ->setRequired(TRUE)
          ->setReadOnly(TRUE);
      }
      else {
        $fields[$entity_type->getKey('bundle')] = BaseFieldDefinition::create('string')
          ->setLabel($entity_type->getBundleLabel())
          ->setRequired(TRUE)
          ->setReadOnly(TRUE);
      }
    }

During the call above, the field definition is a string but the table mapping is for an entity reference. Clearing the caches makes saving work again so I guess something the bundle entity type is somehow getting set and the definition cached somewhere along the road. (I've spent long enough on this already, so won't chase this down further right now).

In any case, the patch helps, though it looks like I will also need to patch menu_item_extras if someone hasn't done that already.

I've updated the merge request for the 10.x branch. This included removing the last commit, which sought to revert the changes removing setting the bundle value.

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

Ok, I'll recheck. Thanks for the report.

When implementing this, please don't forget use cases outside of the USA. In Australia, we have some shorter six digit numbers (132 500, for example) and there are also the emergency service numbers (not necessarily 911 - alternatives I know include 000, 111 and 112.

This is happening - I'm doing a rewrite that is using Context + Media / Display Modes / Response Image Styles (optionally) / Image Styles.

I know it's been ages but thought you might like to know that a rewrite is in progress to use Context + Media. I'll close this issue as from here on, Media will gain a new 'Background Image' bundle with the Context module having support for a reaction type that lets you choose the Media item, display mode and other settings. Since I expect that people won't have a huge number of background images they'll be using, I've made it a dropdown for now. If that expectation turns out to be wrong, I'll implement a different widget.

I'm now rewriting the whole module to use Context + Media to implement the core functionality, and have gotten it going tonight. Plenty of cleanup still to do but wanted to thank you for the idea!

I have a rewrite in progress; the background image entity type won't be used when it's finished (Context reactions instead).

That will be because you're using PHP 8.3. I'm working on a rewrite at the moment and hope to have it available for testing within a week or two - it won't have that issue because I'm developing with PHP 8.3 too, so addressing those issues as part of the upgrade.

I'm working on a rewrite that will use Media support and image styles. I got it working tonight but still have plenty of cleanup to do, upgrade hooks to prepare and tests to write so it will be a while yet. Sorry for the delay but I hope you'll find it much better!

Hi.

Sure. Sorry for my slowness; too many things on the go at once (as always!).

Have you had a chance to test it with D11? I haven't done so thoroughly yet.

Further work is underway to get a D11 version out the door. I haven't dropped the ball :)

Perhaps it would help to use the Context API. For situations like running drush sapi-i, it might help in determining that rendering isn't recursive?

I'd be happy to submit it as "the" solution if that's what people want. Tests have been written; I could add them to the MR and credit my colleage, who wrote them.

And another tweak. Tests are coming.

Here's a small fix to the patch in 77.

I've tried HeaderUtils::parseQuery on the Australian Bureau of Meterology link http://www.bom.gov.au/cgi-bin/wrap_fwo.pl?IDN60140.html (which had two issues - the equals sign being added and the .html being changed to _html). It got through unmangled when extending the patch from #1464244 to replace parse_url in the line above.

Rather than stomping on the existing merge request, I'll attach my version as a patch in the first instance, and put it in the merge request if it gets favourable feedback.

Thanks!

Ah, the spelling isn't a mistake, it's because I'm not an American!

I'll see if I can find time to write the tests but I'm quite likely not to get around to it due to all the other things on my plate, sorry.

Attaching a patch I've prepared that ensures the cleanup is done when there is no data to import.

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

I've reproduced the issue today, finding a problem in the verifyImplementations function in ModuleHandler.php.
The code there invokes function_exists to check that an implementation exists without having first ensured that the file that might contain the function definition has been loaded.
Adding "$this->loadInclude($module, 'module');" at line 700 of ModuleHandler.php (Drupal 10.3.2) makes the problem go away for me.
To reproduce the issue, I'm doing a cache clear, followed by a page load at admin/structure/display-modes/view. The page loads fine the first time but on the second load, I'm getting:

The website encountered an unexpected error. Try again later.

Drupal\Component\Plugin\Exception\InvalidPluginDefinitionException: The "entity_view_mode" entity type did not specify a list_builder handler. in Drupal\Core\Entity\EntityTypeManager->getHandler() (line 265 of core/lib/Drupal/Core/Entity/EntityTypeManager.php).
Drupal\Core\Entity\EntityTypeManager->getListBuilder('entity_view_mode') (Line: 23)
Drupal\Core\Entity\Controller\EntityListController->listing('entity_view_mode')
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 638)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 121)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 181)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 68)
Drupal\simple_oauth\HttpMiddleware\BasicAuthSwap->handle(Object, 1, 1) (Line: 53)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 28)
Drupal\Core\StackMiddleware\ContentLength->handle(Object, 1, 1) (Line: 32)
Drupal\big_pipe\StackMiddleware\ContentLength->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 263)
Drupal\shield\ShieldMiddleware->bypass(Object, 1, 1) (Line: 130)
Drupal\shield\ShieldMiddleware->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 36)
Drupal\Core\StackMiddleware\AjaxPageState->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle(Object, 1, 1) (Line: 741)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

With the change mentioned above in place, I can reload the page without issue.
I assume there's a contrib module or custom code that's the root cause of this, but haven't yet located it. Hopefully the above will at least prove helpful in some way.

Hi.

You'll want to take a look at the way in which your PDF generator is seeking to access the QR code image. It will either be via a file path or a URL. If a URL, perhaps the PDF generator can't resolve the hostname or can't access the URL. If via a file path, configuration such as using a chroot jail or using a relative path and an unanticipated current directory can cause issues like this.

If you can, I'd recommend using the Puphpeteer submodule - you can just point it at a URL and the PDF it generates is exactly what Chrome sees, so things should be more straight forward.

Regards,

Nij

The referenced commit still makes the field a blob for mysql. I tried changing it to a geometry field manually on my local and could do so but was then unable to import data via a feed. I assume more changes are needed for mySQL / mariadb.

You're welcome :)

Our current constraint does allow 6.7.5; it just doesn't require it as a minimum; I'll therefore downgrade the priority and update the minimum.

Thanks for the heads-up.

You're welcome :)

Do you consider the issue resolved? If so, would you please close it? Thanks!

Nigel Cunningham → created an issue.

Ah, it's the new "require bad English" coding standard, starting to bite - among other things. You now have to finish every list of parameters with a comma :(

Preparing a commit that makes PHPCS happy and me sad :)

Hi Mariska.

Thanks for your query.

I've looked at the page you've referenced and tried the button. It isn't using the Printable module but is instead opening the normal browser print dialog for the page. To use Printable, you'll need to configure it under /admin/config/user-interface/printable.

Alternatively, you could make what you currently have work better by adding CSS to your theme that targets print media (more info at https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_media_queries/Printing).

Regards,

Nigel

Hi @skoubi.

There are a couple of update scripts added (printable_update_9000, 9001 and 9002 earlier in the year - the 9002 one is the one that's needed for this iirc.

If not, let me know and I'll take a look - you shouldn't need to apply patches if I've got all the possibilities covered.

Thanks!

I've forgotten the gory details, but it's not rendering the whole thing again - it renders the inner part and then adds the other bits.

Re a new release, sure!

Thanks for the feedback!

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

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

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

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

This is now fixed in the 3.x branch. Apologies for the long time taken in replying.

Merged into 3.x branch, thanks. Used core event in place of rolling our own sanitisation.

Thanks for your patch.

I think I'd prefer to use the core functionality for sanitising filenames, rather than rolling our own, if that's possible. I've found https://www.drupal.org/node/3032541 → and will follow the trail there to see how we can use that sanitisation.

Also, did you run the upgrade script - I think I recall seeing the issue myself and implementing an upgrade hook to address that.

Thanks!

The markup just needed to be a Markup object rather than a raw string - it was getting XSS cleanup applied to it again (it should be safe already because the markup has been generated via rendering once already).

Thanks for your report and help towards debugging the issue.

Sorry for the slow reply; looking at it now.

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

Merged; thanks.

I've reviewed the MR and checked that we don't actually configure the crawler, so don't need to worry about reconfiguring it.

Merging; thanks.

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

You're welcome.

I'll close this issue for now. Hard to pick a 'resolution' but I'll go with "works as designed" since it's not a printable issue. If they do make changes or you want me to revisit the issue, please feel free to reopen it or open another one.

The issue is caused by the fact that the image hotspots module generates the hotspots using Javascript in the browser, rather than as part of the page generation. Doing things that way means (among other things) that there's no way for the Printable module to tell that it needs javascript to run, or to tell that it has run before generating a PDF. For some of the PDF generation techniques, there's also no way for us to run Javascript. The exception is the Puppeteer support, which uses headless Chrome. In that case we do have a way to wait for Javascript to run - I used it at a former employer to wait for pagedJS to do its work.

So, I'm sorry to say this but you'll need to get the image hotspot author(s) to modify their module before you'll be able to use it with printable. Ideally, it would be modified so that the HTML and CSS generated include the hotspots, without any need for JS to run.

I hope that's at least a little bit helpful!

Thanks!

Here's an initial version, tested on a site I'm building with nodes and eck entities so far.

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

Would you be able to provide sample data / setup steps where you're expecting hotspots, so that I can just get on with implementing it and testing it for you, please?

Thanks!

Thanks for the reply.

Confirmed that it works now:

$ composer require drupal/emulsify_twig:^5 drupal/emulsify_drupal:^4.9 -W
./composer.json has been updated
Running composer update drupal/emulsify_twig drupal/emulsify_drupal --with-all-dependencies
Gathering patches for root package.
Loading composer repositories with package information
Updating dependencies
Lock file operations: 0 installs, 2 updates, 0 removals
  - Upgrading drupal/emulsify_drupal (4.9.0 => 4.9.1)
  - Upgrading drupal/emulsify_twig (4.0.0 => 5.0.0)
Writing lock file
Installing dependencies from lock file (including require-dev)
Package operations: 0 installs, 2 updates, 0 removals
  - Downloading drupal/emulsify_drupal (4.9.1)
Gathering patches for root package.
Gathering patches for dependencies. This might take a minute.
  - Upgrading drupal/emulsify_twig (4.0.0 => 5.0.0): Extracting archive
  - Upgrading drupal/emulsify_drupal (4.9.0 => 4.9.1): Extracting archive
Generating autoload files
58 packages you are using are looking for funding.
Use the `composer fund` command to find out more!
No security vulnerability advisories found.

Thanks!

Nij

Nigel Cunningham → created an issue.

Thanks for the issue.

I implemented the API for a project I was working on, which is why puppeteer support got implemented first. If the API was implemented for other plugins as it stands, would it meet your requirements?

I'm thinking of a more general redesign across printable / pdf_api so would appreciate any input you have there.

Nij

Thanks for confirming that.

I'll close the issue now.

Thanks for the report, but that looks to me to be already fixed in the 2.0.x branch. It was fixed in commit 8aae9ad4 on 22 November last year.

Updating the issue title to reflect where the conversation went. Interesting request!

Ok, this is now implemented and in the 2.0.x branch.

I think I'll just make using context the way, going forward - for the moment setting a context overrides the older configuration options. What I'll probably do is make the module just provide a way to configure the images themselves, and have Context be the means by which we decide which background image to use.

I'll leave the issue open until I get the above done and everything nice and clean.

Hi all.

I'll do this by adding support for the Context module, as suggested in issue 3160906.

I'll therefore mark this as a duplicate of that module.

Thanks for your interest and sorry it has taken so long to get started on this.

That sounds like a good idea; starting work on implementing it.

Patch applied; thanks.

Applying the merge request as it stands; it deals with nearly all the complaints.

Thanks for the report and sorry that it has taken so long to respond.

Were you able to identify a set of steps to reproduce the issue?

Tested and merged. Thanks!

Crediting Slashrsm too from the duplicate issue 3368941.

Ah, this is a duplicate of #3446229.

Thanks for your report.

Could I get more detail regarding your configuration on that page? I haven't yet succeeded in reproducing the issue.

Terrific; thanks!

The latest commit should fix the issue for you. Apologies for the inconvenience!

Ah, I see, I was being too blinkered. I'll prepare another patch now and push that.

Apologies!

Please give that latest commit a try - it should fix the issue.

Thanks for opening this issue. I've just pushed some work that I've done over the last few days - as you rightly pointed out, more was needed than just fixing the arguments. I'll credit your work on this when I get up to closing the issue. In the meantime, if you'd like to try the current dev branch, it will hopefully fix issues for you.

I've just pushed my work on this ticket (and related tickets) to the dev branch and would appreciate any test results others can provide.

I'm working on this at the moment - I have it mostly working but one bit that's still problematic. I'm hoping to finish it tomorrow.

Sorry for the slow response fab001, but you do seem to be assuming that we're paid to maintain this code. That's not the case for me at the moment, so I'm putting my effort into seeking a job and seeking to make a job for myself.

Perhaps you could post the change you made, so that others in the same situation will find your issue useful?

Regards,

Nigel

Let's drop support for Drupal 9.4. It's not supported by core anymore so I think we have good justification.

Hi there.

There's a README.md in the root of the module but if you have questions, please go ahead and ask.

I took over maintaining the module a few years ago and haven't done a heap of work on it, but am hoping to make some serious architectural improvements in the near future. If you have thoughts or suggestions, I'd appreciate them.

Regards,

Nigel

Thanks for the reply!

My thinking is that we should support current versions, which would mean 9.5 and 10. Assuming you agree - although I'm maintaining it, I don't think I should be autocratic! - I'd suggest looking at whether we can tell the CI pipeline to use PHP 8 and Drupal 9.5. If it insists on 7, perhaps we should drop testing 9 for now. Does that sound reasonable?

Regards,

Nigel

Hmm, PHP 7 still perhaps?

According to https://www.drupal.org/docs/getting-started/system-requirements/php-requ... → , the only supported version of 9 is 9.5, and that requires PHP 8 so I think we should be safe with them.

Added a patch that checks whether the denominator on the score is zero (unset) and prevents the display of the field if that's the case.