🇦🇺Australia @Nigel Cunningham

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.

Merged; thanks!

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

Why the move away from constructor property promotion?

Thanks! I'll seek to get that issue fixed and merge this.

Hi again.

I've not had success in reproducing the issue. Could I get you to give me a step-by-step, starting with a site install, please?

Thanks!

Thanks for the report.

I'll download the field group module and seek to reproduce the issue.

Thanks. Currently planning how to implement this as part of a bunch of changes.

Looks like this is outdated, but thanks for all your other contributions!

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

I've just done this for the 3.x branch as it stands, will add credit for both of you. Thanks for raising the issue.

I've sought to reproduce this using CKEditor 5 / Drupal 10 and not needed the patch. Could someone please provide more detailed information regarding the circumstances in which it is needed?

Thanks!

I believe this has been implemented but the ticket never got updated. Closing as fixed.

Closing as outdated; the code has moved on too much and PHPCS is passing on all files at the moment.

D9 is now unsupported. Let's close this as Won't fix.

Done in the 3.x branch. I didn't use the patch from hree, sorry (things had moved on too much).

The PR got worked on and merged. I think this can be closed now.

I agree.

I'm planning on implementing a plugin / event based way of altering the content. The link extractor is currently a prime target and modifications like this would fit in well to such a rework.

Hopefully not far away, though I'm checking through the issue queue for other ideas before starting on this.

Rebased to 3.x, updated for changes I've made today and merged. Thanks Chris!

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

This is now implemented and in 3.x dev.

Please test.

Awesome. Thanks for the reply!

Apologies for the long delay in answering.

The theme hooks printable uses work just look other theme hooks so I can only suggest that you follow the normal methods for debugging why your hook isn't being used (eg https://www.drupal.org/docs/develop/theming-drupal/twig-in-drupal/debugg... → ).

We did have a buggy suggestion hook (which I've just fixed) that tried to put printable_ as a prefix for links; it should be on the end if anywhere (since it uses a view mode).

I hope that's helpful.

I'll close this as "Works as designed" but please feel free to comment further if you need help.

Fixed in dev, thanks.

Thanks for the report; this is now fixed in dev.

Thanks for this report.

I've just pushed a series of commits that fix this issue. As part of the fix, they add a new option to the link extractor config form that lets you choose whether to exclude the Printable links from the list. It defaults to enabled for backwards compatibility.

Sorry for the long delay in replying. I'm looking into this now.

Applied.

Nigel Cunningham → created an issue.

Nigel Cunningham → created an issue.